From 86cb66e85808f35c6854d1c2ab9160437b66e3fe Mon Sep 17 00:00:00 2001 From: Molly Struve Date: Thu, 17 Sep 2020 17:20:14 -0500 Subject: [PATCH] [deploy] Feature: Serve Reading List from FeedContent Index (#10294) * Feature:Serve Reading List from FeedContent Index * Fix paging, order ID desc, improve comments --- app/controllers/search_controller.rb | 4 +- .../__tests__/readingList.test.jsx | 2 +- .../readingList/components/ItemListItem.jsx | 4 +- .../__tests__/ItemListItem.test.jsx | 2 +- .../searchableItemList/searchableItemList.js | 1 + .../search/query_builders/feed_content.rb | 10 +- app/services/search/reading_list.rb | 78 ++++++++++++ spec/requests/search_spec.rb | 8 +- spec/services/search/feed_content_spec.rb | 18 +++ spec/services/search/reading_list_spec.rb | 115 ++++++++++++++++++ 10 files changed, 230 insertions(+), 12 deletions(-) create mode 100644 app/services/search/reading_list.rb create mode 100644 spec/services/search/reading_list_spec.rb diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 3bb814152..5e9637713 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -103,8 +103,8 @@ class SearchController < ApplicationController end def reactions - result = Search::Reaction.search_documents( - params: reaction_params.merge(user_id: current_user.id).to_h, + result = Search::ReadingList.search_documents( + params: reaction_params.to_h, user: current_user, ) render json: { result: result["reactions"], total: result["total"] } diff --git a/app/javascript/readingList/__tests__/readingList.test.jsx b/app/javascript/readingList/__tests__/readingList.test.jsx index 359181d7f..32624e3d3 100644 --- a/app/javascript/readingList/__tests__/readingList.test.jsx +++ b/app/javascript/readingList/__tests__/readingList.test.jsx @@ -18,7 +18,7 @@ describe('', () => { body_text: 'Some body text', class_name: 'Article', path: '/bobbytables/what-s-in-your-database-2d3f', - published_date_string: 'Jun 22', + readable_publish_date_string: 'Jun 22', reading_time: 0, tags: [ { diff --git a/app/javascript/readingList/components/ItemListItem.jsx b/app/javascript/readingList/components/ItemListItem.jsx index dbab17077..6de617265 100644 --- a/app/javascript/readingList/components/ItemListItem.jsx +++ b/app/javascript/readingList/components/ItemListItem.jsx @@ -7,7 +7,7 @@ export const ItemListItem = ({ item, children }) => { path: item.reactable.path, title: item.reactable.title, user: item.reactable.user, - publishedDate: item.reactable.published_date_string, + publishedDate: item.reactable.readable_publish_date_string, readingTime: item.reactable.reading_time, tags: item.reactable.tags, }; @@ -59,7 +59,7 @@ const readingListItemPropTypes = PropTypes.shape({ path: PropTypes.string.isRequired, title: PropTypes.string.isRequired, reading_time: PropTypes.number.isRequired, - published_date_string: PropTypes.string.isRequired, + readable_publish_date_string: PropTypes.string.isRequired, user: PropTypes.shape({ username: PropTypes.string.isRequired, profile_image_90: PropTypes.string.isRequired, diff --git a/app/javascript/readingList/components/__tests__/ItemListItem.test.jsx b/app/javascript/readingList/components/__tests__/ItemListItem.test.jsx index 9aeef7d13..ffe05db34 100644 --- a/app/javascript/readingList/components/__tests__/ItemListItem.test.jsx +++ b/app/javascript/readingList/components/__tests__/ItemListItem.test.jsx @@ -9,7 +9,7 @@ function getItem() { reactable: { path: '/article', title: 'Title', - published_date_string: 'Jun 29', + readable_publish_date_string: 'Jun 29', reading_time: 1, user: { username: 'bob', diff --git a/app/javascript/searchableItemList/searchableItemList.js b/app/javascript/searchableItemList/searchableItemList.js index 91486cb96..52dd3b42e 100644 --- a/app/javascript/searchableItemList/searchableItemList.js +++ b/app/javascript/searchableItemList/searchableItemList.js @@ -139,6 +139,7 @@ export function loadNextPage() { const { query, selectedTags, page, statusView } = component.state; component.setState({ page: page + 1 }); component.search(query, { + page: page + 1, tags: selectedTags, statusView, appendItems: true, diff --git a/app/services/search/query_builders/feed_content.rb b/app/services/search/query_builders/feed_content.rb index 4b86e363d..f81bec168 100644 --- a/app/services/search/query_builders/feed_content.rb +++ b/app/services/search/query_builders/feed_content.rb @@ -138,8 +138,14 @@ module Search TERM_KEYS.filter_map do |term_key, search_key| next unless @params.key? term_key - { terms: { search_key => Array.wrap(@params[term_key]) } } - end + values = Array.wrap(@params[term_key]) + + if params[:tag_boolean_mode] == "all" && term_key == :tag_names + values.map { |val| { term: { search_key => val } } } + else + { terms: { search_key => values } } + end + end.compact end def range_keys diff --git a/app/services/search/reading_list.rb b/app/services/search/reading_list.rb new file mode 100644 index 000000000..e57adb59c --- /dev/null +++ b/app/services/search/reading_list.rb @@ -0,0 +1,78 @@ +module Search + # This class does not inherit from Search::Base like our other search classes bc it + # is using the FeedContent index to search for ReadingList reaction articles rather than its + # own separate index. The primary function of this class is to help combine and parse reaction + # data with article Elasticsearch data to populate the Reading List view. + class ReadingList + DEFAULT_PAGE = 0 + DEFAULT_PER_PAGE = 60 + DEFAULT_STATUS = %w[valid confirmed].freeze + + # Using a class method here to follow the pattern of the other Search classes + def self.search_documents(params:, user:) + new(params: params, user: user).reading_list_reactions + end + + def initialize(params:, user:) + self.status = params.delete(:status) || DEFAULT_STATUS + self.view_page = params.delete(:page) || DEFAULT_PAGE + self.view_per_page = params.delete(:per_page) || DEFAULT_PER_PAGE + self.user = user + self.search_params = params + end + + def reading_list_reactions + ordered_articles = parse_and_order_articles(article_docs) + { "reactions" => paginate_articles(ordered_articles), "total" => total } + end + + private + + attr_accessor :search_params, :user, :status, :view_page, :view_per_page, :total + + def paginate_articles(ordered_articles) + start = view_per_page * view_page + ordered_articles[start, view_per_page] || [] + end + + def article_docs + return @article_docs if @article_docs + + # Gather articles from Elasticsearch based on search criteria containing + # tags, text search, status, and the list of IDs of all articles in a user's + # reading list + docs = FeedContent.search_documents( + params: search_params.merge( + id: search_ids, + class_name: "Article", + page: 0, + per_page: reading_list_article_ids.count, + ), + ) + self.total = docs.count + @article_docs = docs.index_by { |doc| doc["id"] } + end + + def reading_list_article_ids + # Collect all reading list IDs and article IDs for a user + @reading_list_article_ids ||= user.reactions.readinglist.where(status: status).order(id: :desc).pluck( + :reactable_id, :id + ).to_h + end + + def search_ids + reading_list_article_ids.keys.map { |id| "article_#{id}" } + end + + def parse_and_order_articles(articles) + # Combines reaction and article data to create hashes that contain the fields + # the reading list view needs. Ensures articles are returned in order of reaction ID + reading_list_article_ids.map do |article_id, reaction_id| + found_article_doc = articles[article_id] + next unless found_article_doc + + { "id" => reaction_id, "user_id" => user.id, "reactable" => articles[article_id] } + end.compact + end + end +end diff --git a/spec/requests/search_spec.rb b/spec/requests/search_spec.rb index f5aec976c..c4e0558d3 100644 --- a/spec/requests/search_spec.rb +++ b/spec/requests/search_spec.rb @@ -133,7 +133,7 @@ RSpec.describe "Search", type: :request, proper_status: true do it "returns json with reactions and total" do sign_in authorized_user - allow(Search::Reaction).to receive(:search_documents).and_return( + allow(Search::ReadingList).to receive(:search_documents).and_return( mock_response, ) get "/search/reactions" @@ -142,13 +142,13 @@ RSpec.describe "Search", type: :request, proper_status: true do it "accepts array of tag names" do sign_in authorized_user - allow(Search::Reaction).to receive(:search_documents).and_return( + allow(Search::ReadingList).to receive(:search_documents).and_return( mock_response, ) get "/search/reactions?tag_names[]=1&tag_names[]=2" - expect(Search::Reaction).to have_received( + expect(Search::ReadingList).to have_received( :search_documents, - ).with(params: { "tag_names" => %w[1 2], "user_id" => authorized_user.id }) + ).with(params: { "tag_names" => %w[1 2] }, user: authorized_user) end end end diff --git a/spec/services/search/feed_content_spec.rb b/spec/services/search/feed_content_spec.rb index 9a5c6bf80..1670603f1 100644 --- a/spec/services/search/feed_content_spec.rb +++ b/spec/services/search/feed_content_spec.rb @@ -116,6 +116,24 @@ RSpec.describe Search::FeedContent, type: :service do expect(doc_ids).to include(article1.id) end + it "filters by multiple tag names when tag_boolean_mode is set to all" do + tag_one = create(:tag) + tag_two = create(:tag) + article1.tags << tag_one + article2.tags << tag_two + article2.tags << tag_one + index_documents([article1, article2]) + query_params = { + tag_names: [tag_one.name, tag_two.name], + tag_boolean_mode: "all" + } + + feed_docs = described_class.search_documents(params: query_params) + expect(feed_docs.count).to eq(1) + doc_ids = feed_docs.map { |t| t["id"] } + expect(doc_ids).to include(article2.id) + end + it "filters by user_id" do index_documents([article1, article2]) query_params = { size: 5, user_id: article1.user_id } diff --git a/spec/services/search/reading_list_spec.rb b/spec/services/search/reading_list_spec.rb new file mode 100644 index 000000000..2295199a5 --- /dev/null +++ b/spec/services/search/reading_list_spec.rb @@ -0,0 +1,115 @@ +require "rails_helper" + +RSpec.describe Search::ReadingList, type: :service do + describe "::search_documents", elasticsearch: "FeedContent" do + let(:user) { create(:user) } + let(:article0) { create(:article) } + let(:article1) { create(:article) } + let(:article2) { create(:article) } + let(:reaction1) { create(:reaction, user: user, category: "readinglist", reactable: article1) } + let(:reaction2) { create(:reaction, user: user, category: "readinglist", reactable: article2) } + let(:reaction3) { create(:reaction, user: user, category: "readinglist", reactable: article0) } + let(:query_params) { {} } + + before do + reaction1 + reaction2 + end + + def index_documents(docs) + index_documents_for_search_class(Array.wrap(docs), Search::FeedContent) + end + + it "returns fields necessary for the view" do + view_keys = %w[id reactable user_id] + reactable_keys = %w[title path readable_publish_date_string tags user] + user_keys = %w[username name profile_image_90] + index_documents(article1) + + result = described_class.search_documents(params: {}, user: user) + reaction_doc = result["reactions"].first + expect(result["total"]).to be_present + expect(reaction_doc.keys).to include(*view_keys) + expect(reaction_doc["reactable"].keys).to include(*reactable_keys) + expect(reaction_doc.dig("reactable", "user").keys).to include(*user_keys) + end + + context "with a query" do + it "searches by search_fields" do + allow(article1).to receive(:title).and_return("ruby") + allow(article2).to receive(:body_text).and_return("Ruby Tuesday") + index_documents([article1, article2]) + query_params[:search_fields] = "ruby" + + reaction_docs = described_class.search_documents(params: query_params, user: user)["reactions"] + expect(reaction_docs.count).to eq(2) + doc_ids = reaction_docs.map { |t| t["id"] } + expect(doc_ids).to include(reaction1.id, reaction2.id) + end + end + + context "with a filter term" do + let(:tag_one) { create(:tag) } + let(:tag_two) { create(:tag) } + + it "filters by tag names" do + article1.tags << tag_one + article2.tags << tag_two + index_documents([article1, article2]) + query_params[:tag_names] = [tag_one.name] + + reaction_docs = described_class.search_documents(params: query_params, user: user)["reactions"] + expect(reaction_docs.count).to eq(1) + doc_ids = reaction_docs.map { |t| t["id"] } + expect(doc_ids).to include(reaction1.id) + expect(doc_ids).not_to include(reaction2.id) + end + + it "filters by multiple tag names when tag_boolean_mode is set to all" do + article1.tags << tag_one + article2.tags << tag_two + article2.tags << tag_one + index_documents([article1, article2]) + query_params[:tag_names] = [tag_one.name, tag_two.name] + query_params[:tag_boolean_mode] = "all" + + reaction_docs = described_class.search_documents(params: query_params, user: user)["reactions"] + expect(reaction_docs.count).to eq(1) + doc_ids = reaction_docs.map { |t| t["id"] } + expect(doc_ids).to include(reaction2.id) + expect(doc_ids).not_to include(reaction1.id) + end + + it "filters by status" do + reaction1.update(status: "invalid") + index_documents([article1, article2]) + query_params[:status] = %w[valid confirmed] + + reaction_docs = described_class.search_documents(params: query_params, user: user)["reactions"] + expect(reaction_docs.count).to eq(1) + doc_ids = reaction_docs.map { |t| t["id"] } + expect(doc_ids).to include(reaction2.id) + end + + it "only returns readinglist reactions" do + reaction1.update(category: "like") + index_documents([article1, article2]) + + reaction_docs = described_class.search_documents(params: query_params, user: user)["reactions"] + expect(reaction_docs.count).to eq(1) + doc_ids = reaction_docs.map { |t| t["id"] } + expect(doc_ids).to include(reaction2.id) + expect(doc_ids).not_to include(reaction1.id) + end + end + + it "sorts by reaction ID DESC" do + reaction3 + index_documents([article0, article1, article2]) + + reaction_docs = described_class.search_documents(params: query_params, user: user)["reactions"] + doc_ids = reaction_docs.map { |t| t["id"] } + expect(doc_ids).to eq([reaction3.id, reaction2.id, reaction1.id]) + end + end +end