From 47624ddfcd66d79fb02bfad9344331aa92dd39d2 Mon Sep 17 00:00:00 2001 From: rhymes Date: Tue, 3 Nov 2020 14:02:17 +0100 Subject: [PATCH] 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 Co-authored-by: Michael Kohl --- app/models/user.rb | 1 + app/services/feeds/import.rb | 15 +++-- .../articles/dev_feeds_import_worker.rb | 10 ++- .../accepts_a_subset_of_users.approved.txt | 1 + ..._given_users_are_without_feed.approved.txt | 1 + spec/services/feeds/import_spec.rb | 62 +++++++++++++------ .../articles/dev_feeds_import_worker_spec.rb | 19 +++++- 7 files changed, 81 insertions(+), 28 deletions(-) create mode 100644 spec/fixtures/approvals/feeds_import/call/with_an_explicit_set_of_users/accepts_a_subset_of_users.approved.txt create mode 100644 spec/fixtures/approvals/feeds_import/call/with_an_explicit_set_of_users/imports_no_articles_if_given_users_are_without_feed.approved.txt diff --git a/app/models/user.rb b/app/models/user.rb index 178748089..0d9e6d8dd 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/services/feeds/import.rb b/app/services/feeds/import.rb index 247f92534..074673e21 100644 --- a/app/services/feeds/import.rb +++ b/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 diff --git a/app/workers/articles/dev_feeds_import_worker.rb b/app/workers/articles/dev_feeds_import_worker.rb index 38118683b..b6bd9186d 100644 --- a/app/workers/articles/dev_feeds_import_worker.rb +++ b/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 diff --git a/spec/fixtures/approvals/feeds_import/call/with_an_explicit_set_of_users/accepts_a_subset_of_users.approved.txt b/spec/fixtures/approvals/feeds_import/call/with_an_explicit_set_of_users/accepts_a_subset_of_users.approved.txt new file mode 100644 index 000000000..9a037142a --- /dev/null +++ b/spec/fixtures/approvals/feeds_import/call/with_an_explicit_set_of_users/accepts_a_subset_of_users.approved.txt @@ -0,0 +1 @@ +10 \ No newline at end of file diff --git a/spec/fixtures/approvals/feeds_import/call/with_an_explicit_set_of_users/imports_no_articles_if_given_users_are_without_feed.approved.txt b/spec/fixtures/approvals/feeds_import/call/with_an_explicit_set_of_users/imports_no_articles_if_given_users_are_without_feed.approved.txt new file mode 100644 index 000000000..c22708346 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/spec/services/feeds/import_spec.rb b/spec/services/feeds/import_spec.rb index d3ef87ce2..fafa954a9 100644 --- a/spec/services/feeds/import_spec.rb +++ b/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 diff --git a/spec/workers/articles/dev_feeds_import_worker_spec.rb b/spec/workers/articles/dev_feeds_import_worker_spec.rb index 0cf5c1387..a3e26bc07 100644 --- a/spec/workers/articles/dev_feeds_import_worker_spec.rb +++ b/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