Favoring User scope as parameter for feed fetching (#16763)

This change makes it easier to resolve forem/forem#16487.

What do I mean by that?  To address forem/forem#16487, I need to only
select user's who are authorized to create articles.  In
forem/forem#16732 I added a method that will allow chaining of a User
scope.  So with this current commit and forem/forem#16732, I'm
triangulating on an approach that will make that change easier.

I did not want to conflate those two, as mixing this PR's change and
what is necessary for the closing PR would create a more complicated
review.  Not unduly complicated, but one that will require more tests
and a change in logic.  And for someone reviewing the diff, those
concerns could easily be lost.

Yes, I have changed the method signature, but that method signature is
limited to one location:

```shell
❯ rg "Feeds::Import.call"

app/workers/feeds/import_articles_worker.rb
21:      ::Feeds::Import.call(users_scope: users_scope, earlier_than: earlier_than)
```

So I look to the method signature a bit as an internal API, hence the change.

Related to forem/forem#16487

app/services/feeds/import.rb

@@ -1,13 +1,24 @@
 module Feeds
+  # Responsible for fetching RSS feeds for multiple users.
+  #
+  # @see {Feeds::Import.call}
   class Import
-    def self.call(users: nil, earlier_than: nil)
-      new(users: users, earlier_than: earlier_than).call
+    # Fetch the feeds for the given users (with some filtering based on internal business logic).
+    #
+    # @param users_scope [ActiveRecord::Relation<User>] the initial scope for determining the users
+    #        whose feeds we'll be fetching.
+    #
+    # @param earlier_than [NilClass, ActiveSupport::TimeWithZone] when given, use this to further
+    #        filter the user's who's articles we'll fetch.  That is to say, we won't fetch anyone's
+    #        feeds who's last fetch time was after our earlier_than parameter.
+    #
+    # @return [Integer] count of total articles fetched.
+    def self.call(users_scope: User, earlier_than: nil)
+      new(users_scope: users_scope, earlier_than: earlier_than).call
     end
 
-    def initialize(users: nil, earlier_than: nil)
-      # using nil here to avoid an unnecessary table count to check presence
-      @users = users || User.where(id: Users::Setting.with_feed.select(:user_id))
-      @earlier_than = earlier_than
+    def initialize(users_scope: User, earlier_than: nil)
+      set_users_from(users_scope: users_scope, earlier_than: earlier_than)
 
       # NOTE: should these be configurable? Currently they are the result of empiric
       # tests trying to find a balance between memory occupation and speed
@@ -52,9 +63,15 @@ module Feeds
 
     private
 
-    attr_reader :earlier_than, :users_batch_size, :num_fetchers, :num_parsers
+    attr_reader :earlier_than, :users_batch_size, :num_fetchers, :num_parsers, :users
+
+    # TODO: jeremyf, the logic around which users is a bit convoluted; now we need to consider Article policies
+    #
+    # using nil here to avoid an unnecessary table count to check presence
+    def set_users_from(users_scope:, earlier_than:)
+      @users = users_scope.where(id: Users::Setting.with_feed.select(:user_id))
+      @earlier_than = earlier_than
 
-    def users
       return @users unless earlier_than
 
       # Filtering users whose feed hasn't been processed in the last `earlier_than` time span.

app/workers/feeds/import_articles_worker.rb

@@ -8,17 +8,17 @@ module Feeds
     # by using YAML to define jobs arguments does not support datetimes evaluated
     # at runtime
     def perform(user_ids = [], earlier_than = nil)
+      users_scope = User
       if user_ids.present?
-        users = User.where(id: user_ids)
+        users_scope = users_scope.where(id: user_ids)
         # we assume that forcing a single import should not take into account
         # the last time a feed was fetched at
         earlier_than = nil
       else
-        users = nil
         earlier_than ||= 4.hours.ago
       end
 
-      ::Feeds::Import.call(users: users, earlier_than: earlier_than)
+      ::Feeds::Import.call(users_scope: users_scope, earlier_than: earlier_than)
     end
   end
 end

spec/services/feeds/import_spec.rb

@@ -104,7 +104,9 @@ RSpec.describe Feeds::Import, type: :service, vcr: true do
     context "with an explicit set of users", vcr: { cassette_name: "feeds_import" } do
       # TODO: We could probably improve these tests by parsing against the items in the feed rather than hardcoding
       it "accepts a subset of users" do
-        num_articles = described_class.call(users: User.where(id: Users::Setting.with_feed.select(:user_id)).limit(1))
+        num_articles = described_class.call(
+          users_scope: User.where(id: Users::Setting.with_feed.select(:user_id)).limit(1),
+        )
 
         expect(num_articles).to eq(10)
       end
@@ -114,7 +116,7 @@ RSpec.describe Feeds::Import, type: :service, vcr: true do
         user.setting.update(feed_url: nil)
 
         # rubocop:disable Layout/LineLength
-        expect(described_class.call(users: User.where(id: Users::Setting.where(feed_url: nil).select(:user_id)))).to eq(0)
+        expect(described_class.call(users_scope: User.where(id: Users::Setting.where(feed_url: nil).select(:user_id)))).to eq(0)
         # rubocop:enable Layout/LineLength
       end
     end
@@ -188,7 +190,7 @@ RSpec.describe Feeds::Import, type: :service, vcr: true do
       user.setting.update(feed_url: "https://medium.com/feed/@dvirsegal")
 
       expect do
-        described_class.call(users: User.where(id: user.id))
+        described_class.call(users_scope: User.where(id: user.id))
       end.to change(user.articles, :count).by(10)
     end
 
@@ -197,7 +199,7 @@ RSpec.describe Feeds::Import, type: :service, vcr: true do
       user.setting.update(feed_url: "https://swimburger.net/atom.xml")
 
       expect do
-        described_class.call(users: User.where(id: user.id))
+        described_class.call(users_scope: User.where(id: user.id))
       end.to change(user.articles, :count).by(10)
 
       body_markdown = user.articles.last.body_markdown
@@ -215,7 +217,9 @@ RSpec.describe Feeds::Import, type: :service, vcr: true do
       rss_feed_user1 = Users::Setting.find_by(feed_url: link).user
       rss_feed_user2 = create(:user)
       rss_feed_user2.setting.update!(feed_url: link)
-      expect { described_class.call(users: User.where(id: Users::Setting.where(feed_url: link).select(:user_id))) }
+      expect do
+        described_class.call(users_scope: User.where(id: Users::Setting.where(feed_url: link).select(:user_id)))
+      end
         .to change(rss_feed_user1.articles, :count).by(10)
         .and change(rss_feed_user2.articles, :count).by(10)
     end
@@ -225,7 +229,9 @@ RSpec.describe Feeds::Import, type: :service, vcr: true do
       rss_feed_user1.setting.update!(feed_mark_canonical: true)
       rss_feed_user2 = create(:user)
       rss_feed_user2.setting.update!(feed_url: link, feed_mark_canonical: true)
-      expect { described_class.call(users: User.where(id: Users::Setting.where(feed_url: link).select(:user_id))) }
+      expect do
+        described_class.call(users_scope: User.where(id: Users::Setting.where(feed_url: link).select(:user_id)))
+      end
         .to change(rss_feed_user1.articles, :count).by(10)
         .and change(rss_feed_user2.articles, :count).by(10)
     end

spec/workers/feeds/import_articles_worker_spec.rb

@@ -12,7 +12,7 @@ RSpec.describe Feeds::ImportArticlesWorker, type: :worker do
       Timecop.freeze(Time.current) do
         worker.perform
 
-        expect(Feeds::Import).to have_received(:call).with(users: nil, earlier_than: 4.hours.ago)
+        expect(Feeds::Import).to have_received(:call).with(users_scope: User, earlier_than: 4.hours.ago)
       end
     end
 
@@ -22,7 +22,7 @@ RSpec.describe Feeds::ImportArticlesWorker, type: :worker do
       Timecop.freeze(Time.current) do
         worker.perform([], 1.minute.ago)
 
-        expect(Feeds::Import).to have_received(:call).with(users: nil, earlier_than: 1.minute.ago)
+        expect(Feeds::Import).to have_received(:call).with(users_scope: User, earlier_than: 1.minute.ago)
       end
     end
 
@@ -33,7 +33,7 @@ RSpec.describe Feeds::ImportArticlesWorker, type: :worker do
 
       worker.perform([user.id])
 
-      expect(Feeds::Import).to have_received(:call).with(users: User.where(id: [user.id]), earlier_than: nil)
+      expect(Feeds::Import).to have_received(:call).with(users_scope: User.where(id: [user.id]), earlier_than: nil)
     end
   end
 end