Add users param to Feeds::Import (#11234)

* Add users param to Feeds::Import

* Fix init and specs

* Replace explicit nil check

Co-authored-by: Michael Kohl <me@citizen428.net>

Co-authored-by: Michael Kohl <me@citizen428.net>

app/models/user.rb

@@ -211,6 +211,7 @@ class User < ApplicationRecord
 
   scope :eager_load_serialized_data, -> { includes(:roles) }
   scope :registered, -> { where(registered: true) }
+  scope :with_feed, -> { where.not(feed_url: [nil, ""]) }
 
   before_validation :check_for_username_change
   before_validation :downcase_email

app/services/feeds/import.rb

@@ -3,13 +3,13 @@
 # => add Feeds::ValidateFeedUrl to validate a single feed URL
 module Feeds
   class Import
-    def self.call
-      new.call
+    def self.call(users: nil)
+      new(users: users).call
     end
 
-    # TODO: add `users` param
-    def initialize
-      @users = User.where.not(feed_url: [nil, ""])
+    def initialize(users: nil)
+      # using nil here to avoid an unnecessary table count to check presence
+      @users = users || User.with_feed
 
       # NOTE: should these be configurable? Currently they are the result of empiric
       # tests trying to find a balance between memory occupation and speed
@@ -56,7 +56,10 @@ module Feeds
       data = batch_of_users.pluck(:id, :feed_url)
 
       result = Parallel.map(data, in_threads: num_fetchers) do |user_id, url|
-        response = HTTParty.get(url.strip, timeout: 10)
+        cleaned_url = url.to_s.strip
+        next if cleaned_url.blank?
+
+        response = HTTParty.get(cleaned_url, timeout: 10)
 
         [user_id, response.body]
       rescue StandardError => e

app/workers/articles/dev_feeds_import_worker.rb

@@ -7,11 +7,17 @@ module Articles
 
     sidekiq_options queue: :medium_priority, retry: 10
 
-    def perform
+    def perform(user_ids = [])
       return unless SiteConfig.community_name == "DEV"
       return if Rails.cache.read("cancel_feeds_import").present?
 
-      ::Feeds::Import.call
+      users = if user_ids.present?
+                User.where(id: user_ids)
+              else
+                User.with_feed
+              end
+
+      ::Feeds::Import.call(users: users)
     end
   end
 end

spec/fixtures/approvals/feeds_import/call/with_an_explicit_set_of_users/accepts_a_subset_of_users.approved.txt

@@ -0,0 +1 @@
+10

spec/fixtures/approvals/feeds_import/call/with_an_explicit_set_of_users/imports_no_articles_if_given_users_are_without_feed.approved.txt

@@ -0,0 +1 @@
+0

spec/services/feeds/import_spec.rb

@@ -63,29 +63,55 @@ RSpec.describe Feeds::Import, type: :service, vcr: true, db_strategy: :truncatio
       end
     end
 
-    # it "reports an article creation error" do
-    #   allow(described_classs).to receive(:create_articles_from_user_feed).and_raise(StandardError)
-    #   allow(Honeybadger).to receive(:notify)
-
-    #   described_class.call
-
-    #   expect(Honeybadger).to have_received(:notify).at_least(:once)
-    # end
-
-    # it "reports a fetching error" do
-    #   allow(rss_reader).to receive(:fetch_feeds).and_raise(StandardError)
-    #   allow(Honeybadger).to receive(:notify)
-
-    #   described_class.call
-
-    #   expect(Honeybadger).to have_received(:notify).at_least(:once)
-    # end
-
     it "queues as many slack messages as there are articles", vcr: { cassette_name: "feeds_import" } do
       old_count = Slack::Messengers::Worker.jobs.count
       num_articles = described_class.call
       expect(Slack::Messengers::Worker.jobs.count).to eq(old_count + num_articles)
     end
+
+    context "when handling errors", vcr: { cassette_name: "feeds_import" } do
+      it "reports an article creation error" do
+        allow(Article).to receive(:create!).and_raise(StandardError)
+        allow(Rails.logger).to receive(:error)
+
+        described_class.call
+
+        expect(Rails.logger).to have_received(:error).at_least(:once)
+      end
+
+      it "reports a fetching error" do
+        allow(HTTParty).to receive(:get).and_raise(StandardError)
+        allow(Rails.logger).to receive(:error)
+
+        described_class.call
+
+        expect(Rails.logger).to have_received(:error).at_least(:once)
+      end
+
+      it "reports a parsing error" do
+        allow(Feedjira).to receive(:parse).and_raise(StandardError)
+        allow(Rails.logger).to receive(:error)
+
+        described_class.call
+
+        expect(Rails.logger).to have_received(:error).at_least(:once)
+      end
+    end
+
+    context "with an explicit set of users", vcr: { cassette_name: "feeds_import" } do
+      it "accepts a subset of users" do
+        num_articles = described_class.call(users: User.with_feed.limit(1))
+
+        verify(format: :txt) { num_articles }
+      end
+
+      it "imports no articles if given users are without feed" do
+        create(:user, feed_url: nil)
+
+        described_class.call(users: User.where(feed_url: nil))
+        verify(format: :txt) { 0 }
+      end
+    end
   end
 
   context "when feed_referential_link is false" do

spec/workers/articles/dev_feeds_import_worker_spec.rb

@@ -15,7 +15,7 @@ RSpec.describe Articles::DevFeedsImportWorker, type: :worker do
       expect(Feeds::Import).not_to have_received(:call)
     end
 
-    it "does not call the RssReader if the cache instructs it to cancel" do
+    it "does not call Feeds::Import if the cache instructs it to cancel" do
       allow(SiteConfig).to receive(:community_name).and_return("DEV")
       allow(Rails.cache).to receive(:read).with("cancel_feeds_import").and_return("true")
 
@@ -26,7 +26,7 @@ RSpec.describe Articles::DevFeedsImportWorker, type: :worker do
       expect(Feeds::Import).not_to have_received(:call)
     end
 
-    it "calls the RssReader to get all articles" do
+    it "calls the Feeds::Import to get all articles" do
       allow(SiteConfig).to receive(:community_name).and_return("DEV")
       allow(Rails.cache).to receive(:read).with("cancel_feeds_import").and_return(nil)
       allow(Feeds::Import).to receive(:call)
@@ -35,5 +35,20 @@ RSpec.describe Articles::DevFeedsImportWorker, type: :worker do
 
       expect(Feeds::Import).to have_received(:call)
     end
+
+    context "with user ids" do
+      before do
+        allow(SiteConfig).to receive(:community_name).and_return("DEV")
+      end
+
+      it "calls Feeds::Import with the correct users if given user ids" do
+        user = create(:user)
+        allow(Feeds::Import).to receive(:call)
+
+        worker.perform([user.id])
+
+        expect(Feeds::Import).to have_received(:call).with(users: User.where(id: [user.id]))
+      end
+    end
   end
 end