From 4e76771867440e18824ffa1d8da2aca41ecded26 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Wed, 2 Mar 2022 16:45:06 -0500 Subject: [PATCH] Favoring User scope as parameter for feed fetching (#16763) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 33 ++++++++++++++----- app/workers/feeds/import_articles_worker.rb | 6 ++-- spec/services/feeds/import_spec.rb | 18 ++++++---- .../feeds/import_articles_worker_spec.rb | 6 ++-- 4 files changed, 43 insertions(+), 20 deletions(-) diff --git a/app/services/feeds/import.rb b/app/services/feeds/import.rb index fc13ba454..6cbecfb54 100644 --- a/app/services/feeds/import.rb +++ b/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] 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. diff --git a/app/workers/feeds/import_articles_worker.rb b/app/workers/feeds/import_articles_worker.rb index 55cf35bb8..71c095075 100644 --- a/app/workers/feeds/import_articles_worker.rb +++ b/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 diff --git a/spec/services/feeds/import_spec.rb b/spec/services/feeds/import_spec.rb index f4dc04467..26d36c40f 100644 --- a/spec/services/feeds/import_spec.rb +++ b/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 diff --git a/spec/workers/feeds/import_articles_worker_spec.rb b/spec/workers/feeds/import_articles_worker_spec.rb index ef5862011..9113867af 100644 --- a/spec/workers/feeds/import_articles_worker_spec.rb +++ b/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