From 9df5f6af497479146284818efa44d9027605235a Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Wed, 24 Nov 2021 13:22:26 -0500 Subject: [PATCH] Ensuring reading list excludes unpublished articles (#15490) * Ensuring reading list excludes unpublished articles Prior to this commit, if a given user adds an article to their reading list then the author unpublishes the article, the article remains in the reading list. When the given user would then "click" on the now unpublished article, they would get a 404 Not Found notice. With this commit, we now exclude unpublished articles from the search results for the reading list. This should also address the issue of attempting to archive the reading list item (because it won't be visible in the listing). One of the behaviors that is expected is if, in the above scenario, the author again publishes the article, that article will again "appear" on the reading list for the given user. Closes #14796 * Updating documentation and adding spec * Addressing pull request feedback Yes, I should've used a scope! And also, no need to test present. Just let it's "truthy"-ness speak for itself! * Update spec/services/search/reading_list_spec.rb Co-authored-by: Jamie Gaskins Co-authored-by: Jamie Gaskins --- app/models/reaction.rb | 4 +++ app/services/search/reading_list.rb | 15 +++++++++- spec/services/search/reading_list_spec.rb | 35 +++++++++++++++++++---- 3 files changed, 48 insertions(+), 6 deletions(-) diff --git a/app/models/reaction.rb b/app/models/reaction.rb index d4c15f92f..83d8744a0 100644 --- a/app/models/reaction.rb +++ b/app/models/reaction.rb @@ -26,6 +26,10 @@ class Reaction < ApplicationRecord counter_culture :user scope :public_category, -> { where(category: PUBLIC_CATEGORIES) } + + # Be wary, this is all things on the reading list, but for an end + # user they might only see readinglist items that are published. + # See https://github.com/forem/forem/issues/14796 scope :readinglist, -> { where(category: "readinglist") } scope :for_articles, ->(ids) { where(reactable_type: "Article", reactable_id: ids) } scope :eager_load_serialized_data, -> { includes(:reactable, :user) } diff --git a/app/services/search/reading_list.rb b/app/services/search/reading_list.rb index ad2005672..a859ccb56 100644 --- a/app/services/search/reading_list.rb +++ b/app/services/search/reading_list.rb @@ -19,6 +19,17 @@ module Search DEFAULT_PER_PAGE = 60 MAX_PER_PAGE = 100 # to avoid querying too many items, we set a maximum amount for a page + # Search for the given user's reading list. + # + # @param user [User] whose reading list are we searching? + # @param term [String] search term for the articles + # @param statuses [Array] what reading list status(es) to filter our search + # @param tags [Array] a list of tags to filter our search + # @param page [Integer] the page in the pagination + # @param per_page [Integer] how many items do we return in our pagination result set + # + # @return [Hash] with keys :items and :total. + # :total is an Integer and :items is an Array of serialized data. def self.search_documents(user, term: nil, statuses: [], tags: [], page: 0, per_page: DEFAULT_PER_PAGE) return {} unless user @@ -58,7 +69,7 @@ module Search } end - def self.find_articles(user:, term:, statuses:, tags:, page:, per_page:) + def self.find_articles(user:, term:, statuses:, tags:, page:, per_page:, only_published: true) # [@jgaskins, @rhymes] as `reactions` is potentially a big table, adding pagination # to an INNER JOIN (eg. `joins(:reactions)`) exponentially decreases the performance, # incrementing query time as the database has to scan all the rows just to discard @@ -75,6 +86,8 @@ module Search "INNER JOIN (#{reaction_query_sql}) reactions ON reactions.reactable_id = articles.id", ) + relation = relation.published if only_published + relation = relation.search_articles(term) if term.present? relation = relation.cached_tagged_with(tags) if tags.any? diff --git a/spec/services/search/reading_list_spec.rb b/spec/services/search/reading_list_spec.rb index b2360c8fa..a00a815d6 100644 --- a/spec/services/search/reading_list_spec.rb +++ b/spec/services/search/reading_list_spec.rb @@ -6,11 +6,20 @@ RSpec.describe Search::ReadingList, type: :service do let(:article_not_in_reading_list) { create(:article) } let(:article_1) { create(:article, with_tags: false) } let(:article_2) { create(:article, with_tags: false) } + let(:unpublished_article) { create(:article) } def extract_from_results(result, attribute) result[:items].pluck(:reactable).pluck(attribute) end + def count_of_published_readling_items_for(user:) + Article.published.where(id: user.reactions.readinglist.map(&:reactable_id)).count + end + + def published_reading_list_item_for(user:) + user.reactions.readinglist.detect { |item| item.reactable.published? } + end + describe "::search_documents" do before do create(:reaction, user: user, reactable: article, category: :readinglist, status: :valid) @@ -47,7 +56,19 @@ RSpec.describe Search::ReadingList, type: :service do 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) + expect(result[:total]).to eq(count_of_published_readling_items_for(user: user)) + end + + context "with an article added to a reading list then unpublished" do + before do + create(:reaction, user: user, reactable: unpublished_article, category: :readinglist, status: :valid) + unpublished_article.update_columns(published: false) + end + + it "does not include the unpublished article" do + result = described_class.search_documents(user) + expect(extract_from_results(result, :path)).not_to include(unpublished_article.path) + end end context "when describing the result format" do @@ -63,7 +84,11 @@ RSpec.describe Search::ReadingList, type: :service 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) + + # The user's reading includes published and unpublished + # articles. We want to make sure that what we found in the + # search is on the user's reading list. + expect(user.reactions.readinglist.map(&:id)).to include(item[:id]) expect(item[:user_id]).to eq(user.id) end @@ -100,14 +125,14 @@ RSpec.describe Search::ReadingList, type: :service do context "when filtering by statuses" do it "returns confirmed items by default" do - item = user.reactions.readinglist.last + item = published_reading_list_item_for(user: user) 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 = published_reading_list_item_for(user: user) item.update_columns(status: :valid) expect(described_class.search_documents(user)[:items].first[:id]).to eq(item.id) @@ -317,7 +342,7 @@ RSpec.describe Search::ReadingList, type: :service do 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) + expect(result[:total]).to eq(count_of_published_readling_items_for(user: user)) end it "returns no items when out of pagination bounds" do