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 <jamie@forem.com>

Co-authored-by: Jamie Gaskins <jamie@forem.com>

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) }

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<String>] what reading list status(es) to filter our search
+    # @param tags [Array<String>] 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<Symbol,Object>] 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?

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