From 8a55c3e88875383c92658a76c260f16ac770a56f Mon Sep 17 00:00:00 2001 From: rhymes Date: Tue, 27 Apr 2021 08:49:47 +0200 Subject: [PATCH] [Search 2.0] Add query filters for profile, organization and tag index pages (#13510) * Re-order test cases to increase tests readability * Add user_id filter for articles query * Add organization_id and tags filters * Add new params to SearchController#feed_content * Add current_user param so that it can be tested for a small pool of users --- app/controllers/search_controller.rb | 47 ++-- app/queries/homepage/articles_query.rb | 31 ++- app/services/homepage/fetch_articles.rb | 19 +- spec/queries/homepage/articles_query_spec.rb | 218 +++++++++++++------ spec/requests/search_spec.rb | 26 ++- 5 files changed, 246 insertions(+), 95 deletions(-) diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index d5c7fb189..49fcb519e 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -126,31 +126,40 @@ class SearchController < ApplicationController def feed_content class_name = feed_params[:class_name].to_s.inquiry + enable_search_2_homepage = ( + class_name.Article? && + feed_params[:search_fields].blank? && + FeatureFlag.enabled?(:search_2_homepage, current_user) + ) + 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.Article? && feed_params[:search_fields].blank? - # homepage - if FeatureFlag.enabled?(:search_2_homepage) - # NOTE: published_at is sent from the frontend in the following ES-friendly format: - # => {"published_at"=>{"gte"=>"2021-04-06T14:53:23Z"}} - published_at_gte = params.dig(:published_at, :gte) - published_at_gte = Time.zone.parse(published_at_gte) if published_at_gte - published_at = published_at_gte ? published_at_gte.. : nil + elsif enable_search_2_homepage + # NOTE: published_at is sent from the frontend in the following ES-friendly format: + # => {"published_at"=>{"gte"=>"2021-04-06T14:53:23Z"}} + published_at_gte = feed_params.dig(:published_at, :gte) + published_at_gte = Time.zone.parse(published_at_gte) if published_at_gte + published_at = published_at_gte ? published_at_gte.. : nil - Homepage::FetchArticles.call( - approved: params[:approved], - published_at: published_at, - sort_by: params[:sort_by], - sort_direction: params[:sort_direction], - page: params[:page], - per_page: params[:per_page], - ) - else - feed_content_search - end + # Despite the name "Homepage", this is used by the following index pages: + # => homepage (default, top week/month/year/infinity, latest) + # => profile page + # => organization page + # => tag index page + Homepage::FetchArticles.call( + approved: feed_params[:approved], + published_at: published_at, + user_id: feed_params[:user_id], + organization_id: feed_params[:organization_id], + tags: feed_params[:tag_names], + sort_by: params[:sort_by], + sort_direction: params[:sort_direction], + page: params[:page], + per_page: params[:per_page], + ) elsif class_name.Comment? && FeatureFlag.enabled?(:search_2_comments) Search::Postgres::Comment.search_documents( page: feed_params[:page], diff --git a/app/queries/homepage/articles_query.rb b/app/queries/homepage/articles_query.rb index 4067dbbbb..519ebecbe 100644 --- a/app/queries/homepage/articles_query.rb +++ b/app/queries/homepage/articles_query.rb @@ -25,12 +25,24 @@ module Homepage # TODO: [@rhymes] change frontend to start from page 1 def initialize( - approved: nil, published_at: nil, sort_by: nil, sort_direction: nil, page: 0, per_page: DEFAULT_PER_PAGE + approved: nil, + published_at: nil, + user_id: nil, + organization_id: nil, + tags: [], + sort_by: nil, + sort_direction: nil, + page: 0, + per_page: DEFAULT_PER_PAGE ) @relation = Article.published.select(*ATTRIBUTES) @approved = approved @published_at = published_at + @user_id = user_id + @organization_id = organization_id + @tags = tags.presence || [] + @sort_by = sort_by @sort_direction = sort_direction @@ -44,13 +56,22 @@ module Homepage private - attr_reader :relation, :approved, :published_at, :sort_by, :sort_direction, :page, :per_page + attr_reader :relation, :approved, :published_at, :user_id, :organization_id, :tags, :sort_by, :sort_direction, + :page, :per_page def filter - return relation if approved.nil? && published_at.blank? - @relation = @relation.where(approved: approved) unless approved.nil? - @relation = @relation.where(published_at: published_at) if published_at + @relation = @relation.where(published_at: published_at) if published_at.present? + @relation = @relation.where(user_id: user_id) if user_id.present? + @relation = @relation.where(organization_id: organization_id) if organization_id.present? + + # as tags are in `OR` mode we can't use ActiveRecord's `.or()` because it + # would put all the previous filters in `OR` mode with tags, but what we need + # is to only consider tags as a `OR` sub-condition + if tags.present? + conditions = tags.map { |tag| relation.sanitize_sql_array(["cached_tag_list LIKE ?", "%#{tag}%"]) } + @relation = @relation.where(conditions.join(" OR ")) + end relation end diff --git a/app/services/homepage/fetch_articles.rb b/app/services/homepage/fetch_articles.rb index 45fe54cb4..cd5fe7cf1 100644 --- a/app/services/homepage/fetch_articles.rb +++ b/app/services/homepage/fetch_articles.rb @@ -1,13 +1,30 @@ +# This is used to populate the following pages: +# => homepage +# => profile page +# => organization page +# => tag index page +# TODO: rename `Homepage::FetchArticles` to something more generic module Homepage class FetchArticles DEFAULT_PER_PAGE = 60 def self.call( - approved: nil, published_at: nil, sort_by: nil, sort_direction: nil, page: 0, per_page: DEFAULT_PER_PAGE + approved: nil, + published_at: nil, + user_id: nil, + organization_id: nil, + tags: [], + sort_by: nil, + sort_direction: nil, + page: 0, + per_page: DEFAULT_PER_PAGE ) articles = Homepage::ArticlesQuery.call( approved: approved, published_at: published_at, + user_id: user_id, + organization_id: organization_id, + tags: tags, sort_by: sort_by, sort_direction: sort_direction, page: page, diff --git a/spec/queries/homepage/articles_query_spec.rb b/spec/queries/homepage/articles_query_spec.rb index cea806bd0..850607229 100644 --- a/spec/queries/homepage/articles_query_spec.rb +++ b/spec/queries/homepage/articles_query_spec.rb @@ -6,100 +6,180 @@ RSpec.describe Homepage::ArticlesQuery, type: :query do expect(described_class.call).to be_a(ActiveRecord::Relation) end - it "does not return draft articles" do - article = create(:article, published: false, published_at: nil) - - expect(described_class.call.ids).not_to include(article.id) - end - - it "returns both approved and unapproved articles by default" do - approved_article = create(:article, approved: true) - unapproved_article = create(:article, approved: false) - - expected_result = [approved_article.id, unapproved_article.id] - expect(described_class.call.ids).to match_array(expected_result) - end - - it "returns approved articles", :aggregate_failures do - approved_article = create(:article, approved: true) - unapproved_article = create(:article, approved: false) - - result = described_class.call(approved: true).ids - expect(result).to include(approved_article.id) - expect(result).not_to include(unapproved_article.id) - end - - it "returns unapproved articles" do - approved_article = create(:article, approved: true) - unapproved_article = create(:article, approved: false) - - result = described_class.call(approved: false).ids - expect(result).not_to include(approved_article.id) - expect(result).to include(unapproved_article.id) - end - it "returns only published articles" do article = create(:article) expect(described_class.call.ids).to eq([article.id]) end - it "paginates by default" do - stub_const("Homepage::ArticlesQuery::DEFAULT_PER_PAGE", 1) + it "does not return draft articles" do + article = create(:article, published: false, published_at: nil) - create_list(:article, 2) - - expect(described_class.call.size).to eq(1) + expect(described_class.call.ids).not_to include(article.id) end - it "supports pagination params" do - create_list(:article, 2) + describe "approved" do + it "returns both approved and unapproved articles by default" do + approved_article = create(:article, approved: true) + unapproved_article = create(:article, approved: false) - expect(described_class.call(page: 1, per_page: 1).size).to eq(1) + expected_result = [approved_article.id, unapproved_article.id] + expect(described_class.call.ids).to match_array(expected_result) + end + + it "returns approved articles", :aggregate_failures do + approved_article = create(:article, approved: true) + unapproved_article = create(:article, approved: false) + + result = described_class.call(approved: true).ids + expect(result).to include(approved_article.id) + expect(result).not_to include(unapproved_article.id) + end + + it "returns unapproved articles" do + approved_article = create(:article, approved: true) + unapproved_article = create(:article, approved: false) + + result = described_class.call(approved: false).ids + expect(result).not_to include(approved_article.id) + expect(result).to include(unapproved_article.id) + end end - it "filters by publication date", :aggregate_failures do - article = create(:article) + describe "published_at" do + it "filters by publication date", :aggregate_failures do + article = create(:article) - expect(described_class.call(published_at: article.published_at).size).to eq(1) - expect(described_class.call(published_at: 1.month.ago..).size).to eq(1) - expect(described_class.call(published_at: 1.month.from_now)).to be_empty + expect(described_class.call(published_at: nil).size).to eq(1) + expect(described_class.call(published_at: article.published_at).size).to eq(1) + expect(described_class.call(published_at: 1.month.ago..).size).to eq(1) + expect(described_class.call(published_at: 1.month.from_now)).to be_empty + end end - it "sorts by the hotness_score", :aggregate_failures do - article1, article2 = create_list(:article, 2) + describe "user_id" do + it "returns no articles if the user id does not exist" do + expect(described_class.call(user_id: 9999)).to be_empty + end - article1.update_columns(hotness_score: 1) - article2.update_columns(hotness_score: 2) + it "filters articles belonging to the given user id", :aggregate_failures do + article_user1 = create(:article) + article_user2 = create(:article, user: create(:user)) - result = described_class.call(sort_by: :hotness_score, sort_direction: :desc).ids - expect(result).to eq([article2.id, article1.id]) - - result = described_class.call(sort_by: :hotness_score, sort_direction: :asc).ids - expect(result).to eq([article1.id, article2.id]) + expect(described_class.call(user_id: article_user1.user_id).ids).to include(article_user1.id) + expect(described_class.call(user_id: article_user1.user_id).ids).not_to include(article_user2.id) + end end - it "sorts by the public_reactions_count" do - article1, article2 = create_list(:article, 2) + describe "organization_id" do + it "returns no articles if the organization id does not exist" do + expect(described_class.call(organization_id: 9999)).to be_empty + end - article1.update_columns(public_reactions_count: 1) - article2.update_columns(public_reactions_count: 2) + it "filters articles belonging to the given organization id", :aggregate_failures do + org1 = create(:organization) + org2 = create(:organization) + article_org1 = create(:article, organization: org1) + article_org2 = create(:article, organization: org2) + article_no_org = create(:article) - result = described_class.call(sort_by: :public_reactions_count, sort_direction: :desc).ids - expect(result).to eq([article2.id, article1.id]) - - result = described_class.call(sort_by: :public_reactions_count, sort_direction: :asc).ids - expect(result).to eq([article1.id, article2.id]) + expect(described_class.call(organization_id: org1.id).ids).to include(article_org1.id) + expect(described_class.call(organization_id: org1.id).ids).not_to include(article_org2.id) + expect(described_class.call(organization_id: org1.id).ids).not_to include(article_no_org.id) + end end - it "does not sort by unknown parameters" do - article1, article2 = create_list(:article, 2) + describe "tags" do + let(:article1) { create(:article, with_tags: false) } + let(:article2) { create(:article, with_tags: false) } - article1.update_columns(comments_count: 1) - article2.update_columns(comments_count: 2) + it "returns no articles if none of the tags match" do + article1.tag_list.add(:beginners) + article1.save + article2.tag_list.add(:beginners) + article2.save - result = described_class.call(sort_by: :comments_count, sort_direction: :desc).ids - expect(result).not_to eq([article2.id, article1.id]) + expect(described_class.call(tags: [:ruby])).to be_empty + end + + it "filters articles matching the tag" do + article1.tag_list.add(:beginners) + article1.save + + expect(described_class.call(tags: [:beginners]).ids).to eq([article1.id]) + end + + it "filters any article matching any of the tags in the params", :aggregate_failures do + article1.tag_list.add(:beginners) + article1.save + article2.tag_list.add(:ruby) + article2.save + + expect(described_class.call(tags: %i[beginners python]).ids).to eq([article1.id]) + end + + it "filters all articles match any of the tags in the params" do + article1.tag_list.add(:beginners) + article1.save + article2.tag_list.add(:ruby) + article2.save + + expect(described_class.call(tags: %i[beginners ruby]).ids).to match_array([article1.id, article2.id]) + end + end + + describe "pagination" do + it "paginates by default" do + stub_const("Homepage::ArticlesQuery::DEFAULT_PER_PAGE", 1) + + create_list(:article, 2) + + expect(described_class.call.size).to eq(1) + end + + it "supports pagination params" do + create_list(:article, 2) + + expect(described_class.call(page: 1, per_page: 1).size).to eq(1) + end + end + + describe "sorting" do + it "sorts by the hotness_score", :aggregate_failures do + article1, article2 = create_list(:article, 2) + + article1.update_columns(hotness_score: 1) + article2.update_columns(hotness_score: 2) + + result = described_class.call(sort_by: :hotness_score, sort_direction: :desc).ids + expect(result).to eq([article2.id, article1.id]) + + result = described_class.call(sort_by: :hotness_score, sort_direction: :asc).ids + expect(result).to eq([article1.id, article2.id]) + end + + it "sorts by the public_reactions_count" do + article1, article2 = create_list(:article, 2) + + article1.update_columns(public_reactions_count: 1) + article2.update_columns(public_reactions_count: 2) + + result = described_class.call(sort_by: :public_reactions_count, sort_direction: :desc).ids + expect(result).to eq([article2.id, article1.id]) + + result = described_class.call(sort_by: :public_reactions_count, sort_direction: :asc).ids + expect(result).to eq([article1.id, article2.id]) + end + + it "does not sort by unknown parameters" do + article1, article2 = create_list(:article, 2) + + article1.update_columns(comments_count: 1) + article2.update_columns(comments_count: 2) + + result = described_class.call(sort_by: :comments_count, sort_direction: :desc).ids + expect(result).not_to eq([article2.id, article1.id]) + end end end end diff --git a/spec/requests/search_spec.rb b/spec/requests/search_spec.rb index 083557e75..5a1085c1a 100644 --- a/spec/requests/search_spec.rb +++ b/spec/requests/search_spec.rb @@ -235,7 +235,7 @@ RSpec.describe "Search", type: :request, proper_status: true do context "when using PostgreSQL for the homepage" do before do - allow(FeatureFlag).to receive(:enabled?).with(:search_2_homepage).and_return(true) + allow(FeatureFlag).to receive(:enabled?).with(:search_2_homepage, anything).and_return(true) end it "does not call Homepage::FetchArticles when class_name is Article with a search term", :aggregate_failures do @@ -274,6 +274,30 @@ RSpec.describe "Search", type: :request, proper_status: true do get search_feed_content_path(class_name: "Article", published_at: { gte: datetime.iso8601 }) expect(response.parsed_body["result"]).to be_empty end + + it "supports the user_id parameter" do + allow(Homepage::FetchArticles).to receive(:call) + + get search_feed_content_path(class_name: "Article", user_id: 1) + + expect(Homepage::FetchArticles).to have_received(:call).with(hash_including(user_id: "1")) + end + + it "supports the organization_id parameter" do + allow(Homepage::FetchArticles).to receive(:call) + + get search_feed_content_path(class_name: "Article", organization_id: 1) + + expect(Homepage::FetchArticles).to have_received(:call).with(hash_including(organization_id: "1")) + end + + it "supports the tag_names parameter" do + allow(Homepage::FetchArticles).to receive(:call) + + get search_feed_content_path(class_name: "Article", tag_names: %i[ruby]) + + expect(Homepage::FetchArticles).to have_received(:call).with(hash_including(tags: %w[ruby])) + end end context "when using PostgreSQL for comments" do