maverick/docbrown
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Remove RssReader (#12169)

Browse source 
* Add script to remove feature flag

* Remove :feeds_import feature flag and RssReader and related classes

* Change Feeds::ImportArticlesWorker signature to support Sidekiq Cron serialization

* Replace RssReader::Assembler with Feeds::AssembleArticleMarkdown

* Removing Assembler
This commit is contained in:
rhymes 2021-01-08 17:45:15 +01:00 • committed by GitHub
parent a7376ad10d
commit e7f9735354
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
20 changed files with 75 additions and 512 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
app/controllers/users_controller.rb
View file

@ -336,11 +336,7 @@ class UsersController < ApplicationController
def import_articles_from_feed(user)
return if user.feed_url.blank?
if FeatureFlag.enabled?(:feeds_import)
Feeds::ImportArticlesWorker.perform_async(nil, user.id)
else
RssReaderFetchUserWorker.perform_async(user.id)
end
Feeds::ImportArticlesWorker.perform_async(nil, user.id)
end
def profile_params

6
app/models/user.rb
View file

@ -622,11 +622,7 @@ class User < ApplicationRecord
def validate_feed_url
return if feed_url.blank?
valid = if FeatureFlag.enabled?(:feeds_import)
Feeds::ValidateUrl.call(feed_url)
else
RssReader.new.valid_feed_url?(feed_url)
end
valid = Feeds::ValidateUrl.call(feed_url)
errors.add(:feed_url, "is not a valid RSS/Atom feed") unless valid
rescue StandardError => e

2
app/services/feature_flag.rb
View file

@ -1,6 +1,6 @@
module FeatureFlag
class << self
delegate :disable, :enable, :enabled?, :exist?, to: Flipper
delegate :disable, :enable, :enabled?, :exist?, :remove, to: Flipper
def accessible?(feature_flag_name, *args)
feature_flag_name.blank? || !exist?(feature_flag_name) || enabled?(feature_flag_name, *args)

8
app/services/rss_reader/assembler.rb → app/services/feeds/assemble_article_markdown.rb
View file

@ -1,7 +1,7 @@
class RssReader
class Assembler
module Feeds
class AssembleArticleMarkdown
def self.call(item, user, feed, feed_source_url)
new(item, user, feed, feed_source_url).assemble
new(item, user, feed, feed_source_url).call
end
def initialize(item, user, feed, feed_source_url)
@ -13,7 +13,7 @@ class RssReader
@feed_source_url = feed_source_url
end
def assemble
def call
body = <<~HEREDOC
---
title: #{@title}

8
app/services/feeds/import.rb
View file

@ -29,8 +29,8 @@ module Feeds
# NOTE: doing this sequentially to avoid locking problems with the DB
# and unnecessary conflicts
articles = feedjira_objects.flat_map do |user_id, feed|
# TODO: replace `feed` with `feed.url` as `RssReader::Assembler`
# only actually needs feed.url
# TODO: replace `feed` with `feed.url` as `Feeds::AssembleArticleMarkdown`
# only actually uses feed.url
user = batch_of_users.detect { |u| u.id == user_id }
DatadogStatsClient.time("feeds::import::create_articles_from_user_feed", tags: ["user_id:#{user_id}"]) do
@ -118,7 +118,7 @@ module Feeds
result.compact.to_h
end
# TODO: currently this is exactly as in RSSReader, but we might find
# TODO: currently this is exactly as it was in the RssReader, but we might find
# avenues for optimization, like:
# 1. why are we sending N exists query to the DB, one per each item, can we fetch them all?
# 2. should we queue a batch of workers to create articles, but then, following issues ensue:
@ -137,7 +137,7 @@ module Feeds
user_id: user.id,
published_from_feed: true,
show_comments: true,
body_markdown: RssReader::Assembler.call(item, user, feed, feed_source_url),
body_markdown: Feeds::AssembleArticleMarkdown.call(item, user, feed, feed_source_url),
organization_id: nil,
)

100
app/services/rss_reader.rb
View file

@ -1,100 +0,0 @@
class RssReader
def self.get_all_articles(force: true)
new.get_all_articles(force: force)
end
def get_all_articles(force: true)
articles = []
User.where.not(feed_url: [nil, ""]).find_each do |user|
# unless forced, fetch sparingly
next if force == false && (rand(2) == 1 || user.feed_fetched_at > 15.minutes.ago)
user_articles = create_articles_for_user(user)
articles.concat(user_articles) if user_articles
end
articles
end
def fetch_user(user)
create_articles_for_user(user)
end
def valid_feed_url?(link)
true if fetch_rss(link)
rescue StandardError
false
end
private
def create_articles_for_user(user)
user.update_column(:feed_fetched_at, Time.current)
feed = fetch_rss(user.feed_url.strip)
articles = []
feed.entries.reverse_each do |item|
article = make_from_rss_item(item, user, feed)
articles.append(article)
rescue StandardError => e
report_error(
e,
rss_reader_info: {
username: user.username,
feed_url: user.feed_url,
item_count: item_count_error(feed),
error: "RssReaderError: occurred while creating article #{item.url}"
},
)
end
articles
rescue StandardError => e
report_error(
e,
rss_reader_info: {
username: user.username,
feed_url: user.feed_url,
item_count: item_count_error(feed),
error_message: "RssReaderError: occurred while fetching feed"
},
)
[]
end
def fetch_rss(url)
xml = HTTParty.get(url).body
Feedjira.parse xml
end
def make_from_rss_item(item, user, feed)
return if Feeds::CheckItemMediumReply.call(item) || Feeds::CheckItemPreviouslyImported.call(item, user)
feed_source_url = item.url.strip.split("?source=")[0]
article = Article.create!(
feed_source_url: feed_source_url,
user_id: user.id,
published_from_feed: true,
show_comments: true,
body_markdown: RssReader::Assembler.call(item, user, feed, feed_source_url),
organization_id: nil,
)
Slack::Messengers::ArticleFetchedFeed.call(article: article)
article
end
def report_error(error, metadata)
Honeybadger.context(metadata)
Honeybadger.notify(error)
end
def item_count_error(feed)
return "NIL FEED, INVALID URL" unless feed
feed.entries ? feed.entries.length : "no count"
end
end

21
app/workers/articles/rss_reader_worker.rb
View file

@ -1,21 +0,0 @@
module Articles
class RssReaderWorker
include Sidekiq::Worker
sidekiq_options queue: :medium_priority, retry: 10
def perform
# Temporary
# @sre:mstruve This is temporary until we have an efficient way to handle this job
# for our large DEV community. Smaller Forems should be able to handle it no problem
return if SiteConfig.dev_to?
if FeatureFlag.enabled?(:feeds_import)
::Feeds::ImportArticlesWorker.perform_async(4.hours.ago)
else
# don't force fetch. Fetch "random" subset instead of all of them.
::RssReader.get_all_articles(force: false)
end
end
end
end

15
app/workers/feeds/import_articles_worker.rb
View file

@ -4,8 +4,19 @@ module Feeds
sidekiq_options queue: :medium_priority, retry: 10
def perform(earlier_than, user_ids = [])
users = user_ids.present? ? User.where(id: user_ids) : nil
# NOTE: [@rhymes] we need to default earlier_than to `nil` because sidekiq-cron,
# by using YAML to define jobs arguments does not support datetimes evaluated
# at runtime
def perform(earlier_than = nil, user_ids = [])
if user_ids.present?
users = User.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)
end

11
app/workers/rss_reader_fetch_user_worker.rb
View file

@ -1,11 +0,0 @@
class RssReaderFetchUserWorker
include Sidekiq::Worker
sidekiq_options queue: :medium_priority, lock: :until_executed
def perform(user_id)
user = User.find_by(id: user_id)
RssReader.new.fetch_user(user) if user&.feed_url.present?
end
end

2
config/initializers/honeybadger.rb
View file

@ -35,7 +35,7 @@ Honeybadger.configure do |config|
config.breadcrumbs.enabled = true
config.before_notify do |notice|
notice.fingerprint = if notice.error_message&.include?("SIGTERM") && notice.component&.include?("fetch_all_rss")
notice.fingerprint = if notice.error_message&.include?("SIGTERM") && notice.component&.include?("feeds_import")
notice.error_message
elsif (msg_key = MESSAGE_FINGERPRINTS.keys.detect do |k, _v|
notice.error_message&.include?(k)

4
config/schedule.yml
View file

@ -1,6 +1,6 @@
fetch_all_rss:
feeds_import:
cron: "20 * * * *" # every hour, 20 min after the hour
class: "Articles::RssReaderWorker"
class: "Feeds::ImportArticlesWorker"
log_worker_queue_stats:
cron: "*/10 * * * *" # every 10 minutes
class: "Metrics::RecordBackgroundQueueStatsWorker"

7
lib/data_update_scripts/20210107151449_remove_feeds_import_feature_flag.rb Normal file
View file

@ -0,0 +1,7 @@
module DataUpdateScripts
class RemoveFeedsImportFeatureFlag
def run
FeatureFlag.remove(:feeds_import)
end
end
end

15
lib/tasks/fetch.rake
View file

@ -1,18 +1,3 @@
desc "This task is called by the Heroku scheduler add-on"
task fetch_all_rss: :environment do
if FeatureFlag.enabled?(:feeds_import)
Feeds::Import.call(earlier_than: 4.hours.ago)
else
# don't force fetch. Fetch "random" subset instead of all of them.
RssReader.get_all_articles(force: false)
end
end
task fetch_feeds_import: :environment do
Feeds::Import.call(earlier_than: 4.hours.ago)
end
# Temporary
# @sre:mstruve This is temporary until we have an efficient way to handle this task
# in Sidekiq for our large DEV community.

20
spec/lib/data_update_scripts/remove_feeds_import_feature_flag_spec.rb Normal file
View file

@ -0,0 +1,20 @@
require "rails_helper"
require Rails.root.join(
"lib/data_update_scripts/20210107151449_remove_feeds_import_feature_flag.rb",
)
describe DataUpdateScripts::RemoveFeedsImportFeatureFlag do
it "removes the :feeds_import flag" do
FeatureFlag.enable(:feeds_import)
described_class.new.run
expect(FeatureFlag.exist?(:feeds_import)).to be(false)
end
it "works if the flag is not available" do
described_class.new.run
expect(FeatureFlag.exist?(:feeds_import)).to be(false)
end
end

26
spec/models/user_spec.rb
View file

@ -278,31 +278,7 @@ RSpec.describe User, type: :model do
expect(limiter).to have_received(:track_limit_by_action).with(:user_update).twice
end
context "when validating feed_url with RSSReader", vcr: true do
it "is valid with no feed_url" do
user.feed_url = nil
expect(user).to be_valid
end
it "is not valid with an invalid feed_url", vcr: { cassette_name: "feeds_validate_url_invalid" } do
user.feed_url = "http://example.com"
expect(user).not_to be_valid
end
it "is valid with a valid feed_url", vcr: { cassette_name: "feeds_import_medium_vaidehi" } do
user.feed_url = "https://medium.com/feed/@vaidehijoshi"
expect(user).to be_valid
end
end
context "with Feeds::ValidateUrl" do
before do
allow(FeatureFlag).to receive(:enabled?).with(:feeds_import).and_return(true)
end
context "when validating feed_url", vcr: true do
it "is valid with no feed_url" do
user.feed_url = nil

15
spec/requests/user/user_settings_spec.rb
View file

@ -308,25 +308,12 @@ RSpec.describe "UserSettings", type: :request do
let(:feed_url) { "https://medium.com/feed/@vaidehijoshi" }
let(:user) { create(:user, feed_url: feed_url) }
it "invokes RssReaderFetchUserWorker" do
it "invokes Feeds::ImportArticlesWorker" do
allow(Feeds::ImportArticlesWorker).to receive(:perform_async).with(nil, user.id)
allow(RssReaderFetchUserWorker).to receive(:perform_async).with(user.id)
put user_path(user.id), params: { user: { feed_url: feed_url } }
expect(Feeds::ImportArticlesWorker).not_to have_received(:perform_async)
expect(RssReaderFetchUserWorker).to have_received(:perform_async).with(user.id)
end
it "invokes Feeds::ImportArticlesWorker if feeds_import feature flag is on" do
allow(Feeds::ImportArticlesWorker).to receive(:perform_async).with(nil, user.id)
allow(RssReaderFetchUserWorker).to receive(:perform_async).with(user.id)
allow(FeatureFlag).to receive(:enabled?).with(:feeds_import).and_return(true)
put user_path(user.id), params: { user: { feed_url: feed_url } }
expect(Feeds::ImportArticlesWorker).to have_received(:perform_async).with(nil, user.id)
expect(RssReaderFetchUserWorker).not_to have_received(:perform_async).with(user.id)
end
end
end

191
spec/services/rss_reader_spec.rb
View file

@ -1,191 +0,0 @@
require "rails_helper"
require "rss"
RSpec.describe RssReader, type: :service, vcr: true, db_strategy: :truncation do
self.use_transactional_tests = false
let(:link) { "https://medium.com/feed/@vaidehijoshi" }
let(:nonmedium_link) { "https://circleci.com/blog/feed.xml" }
let(:nonpermanent_link) { "https://medium.com/feed/@macsiri/" }
let!(:rss_reader) { described_class.new }
describe "#get_all_articles" do
before do
[link, nonmedium_link, nonpermanent_link].each do |feed_url|
create(:user, feed_url: feed_url)
end
end
it "fetch only articles from a feed_url", vcr: { cassette_name: "rss_reader_fetch_articles" } do
articles = rss_reader.get_all_articles
# the result within the approval file depends on the feed
# not fetching comments is baked into this
verify(format: :txt) { articles.length }
end
it "does not recreate articles if they already exist", vcr: { cassette_name: "rss_reader_fetch_articles_twice" } do
rss_reader.get_all_articles
expect { rss_reader.get_all_articles }.not_to change(Article, :count)
end
it "parses correctly", vcr: { cassette_name: "rss_reader_fetch_articles" } do
rss_reader.get_all_articles
verify format: :txt do
User.find_by(feed_url: nonpermanent_link).articles.first.body_markdown
end
end
it "sets feed_fetched_at to the current time", vcr: { cassette_name: "rss_reader_fetch_articles" } do
Timecop.freeze(Time.current) do
rss_reader.get_all_articles
user = User.find_by(feed_url: nonpermanent_link)
feed_fetched_at = user.feed_fetched_at
expect(feed_fetched_at.to_i).to eq(Time.current.to_i)
end
end
it "does refetch same user over and over by default", vcr: { cassette_name: "rss_reader_fetch_multiple_times" } do
user = User.find_by(feed_url: nonpermanent_link)
Timecop.freeze(Time.current) do
user.update_columns(feed_fetched_at: Time.current)
fetched_at_time = user.reload.feed_fetched_at
# travel a few seconds in the future to simulate a new time
3.times do |i|
Timecop.travel((i + 5).seconds.from_now) do
rss_reader.get_all_articles
end
end
expect(user.reload.feed_fetched_at > fetched_at_time).to be(true)
end
end
it "reports an article creation error" do
allow(rss_reader).to receive(:make_from_rss_item).and_raise(StandardError)
allow(Honeybadger).to receive(:notify)
rss_reader.get_all_articles
expect(Honeybadger).to have_received(:notify).at_least(:once)
end
it "reports a fetching error" do
allow(rss_reader).to receive(:fetch_rss).and_raise(StandardError)
allow(Honeybadger).to receive(:notify)
rss_reader.get_all_articles
expect(Honeybadger).to have_received(:notify).at_least(:once)
end
it "queues as many slack messages as there are articles", vcr: { cassette_name: "rss_reader_fetch_articles" } do
old_count = Slack::Messengers::Worker.jobs.count
articles = rss_reader.get_all_articles
expect(Slack::Messengers::Worker.jobs.count).to eq(old_count + articles.length)
end
end
context "when feed_referential_link is false" do
it "does not self-reference links for user" do
# Article.find_by is used by find_and_replace_possible_links!
# checking its invocation is a shortcut to testing the functionality.
allow(Article).to receive(:find_by).and_call_original
create(:user, feed_url: nonpermanent_link, feed_referential_link: false)
rss_reader.get_all_articles
expect(Article).not_to have_received(:find_by)
end
end
describe "#fetch_user", vcr: { cassette_name: "rss_reader_fetch_medium_feed" } do
before do
[link, nonmedium_link, nonpermanent_link].each do |feed_url|
create(:user, feed_url: feed_url)
end
end
it "gets articles for user" do
articles = rss_reader.fetch_user(User.find_by(feed_url: link))
# the result within the approval file depends on the feed
verify(format: :txt) { articles.length }
end
it "does not set featured_number" do
user = User.find_by(feed_url: link)
rss_reader.fetch_user(user)
expect(user.articles.select(&:featured_number)).to be_empty
end
it "reports an article creation error on the standard logger" do
allow(rss_reader).to receive(:make_from_rss_item).and_raise(StandardError)
allow(Honeybadger).to receive(:notify)
rss_reader.fetch_user(User.find_by(feed_url: link))
expect(Honeybadger).to have_received(:notify).at_least(:once)
end
it "reports a fetching error on the standard logger" do
allow(rss_reader).to receive(:fetch_rss).and_raise(StandardError)
allow(Honeybadger).to receive(:notify)
rss_reader.fetch_user(User.find_by(feed_url: link))
expect(Honeybadger).to have_received(:notify).at_least(:once)
end
it "queues as many slack messages as there are user articles" do
old_count = Slack::Messengers::Worker.jobs.count
articles = rss_reader.fetch_user(User.find_by(feed_url: link))
expect(Slack::Messengers::Worker.jobs.count).to eq(old_count + articles.length)
end
end
describe "#valid_feed_url?" do
it "returns true on valid feed url" do
expect(rss_reader.valid_feed_url?(link)).to be(true)
end
it "returns false on invalid feed url" do
bad_link = "www.google.com"
expect(rss_reader.valid_feed_url?(bad_link)).to be(false)
end
end
describe "feeds parsing and regressions" do
it "parses https://medium.com/feed/@dvirsegal correctly", vcr: { cassette_name: "rss_reader_dvirsegal" } do
user = create(:user, feed_url: "https://medium.com/feed/@dvirsegal")
expect do
rss_reader.fetch_user(user)
end.to change(user.articles, :count).by(10)
end
it "converts/replaces <picture> tags to <img>", vcr: { cassette_name: "rss_reader_swimburger" } do
user = create(:user, feed_url: "https://swimburger.net/atom.xml")
expect do
rss_reader.fetch_user(user)
end.to change(user.articles, :count).by(10)
body_markdown = user.articles.last.body_markdown
expect(body_markdown).not_to include("<picture>")
expected_image_markdown =
"![Screenshot of Azure left navigation pane](https://swimburger.net/media/lxypkhak/azure-create-a-resource.png)"
expect(body_markdown).to include(expected_image_markdown)
end
end
end

51
spec/workers/articles/rss_reader_worker_spec.rb
View file

@ -1,51 +0,0 @@
require "rails_helper"
RSpec.describe Articles::RssReaderWorker, type: :worker do
let(:worker) { subject }
include_examples "#enqueues_on_correct_queue", "medium_priority"
describe "#perform" do
it "instructs RssReader to fetch all articles" do
allow(RssReader).to receive(:get_all_articles)
worker.perform
expect(RssReader).to have_received(:get_all_articles).with(force: false)
end
it "does not enqueue Feeds::ImportArticlesWorker if the :feeds_import flag is not enabled" do
allow(RssReader).to receive(:get_all_articles)
sidekiq_assert_no_enqueued_jobs do
worker.perform
end
expect(RssReader).to have_received(:get_all_articles).with(force: false)
end
it "enqueues Feeds::ImportArticlesWorker if the :feeds_import flag is enabled" do
allow(Feeds::ImportArticlesWorker).to receive(:perform_async)
allow(RssReader).to receive(:get_all_articles)
allow(FeatureFlag).to receive(:enabled?).with(:feeds_import).and_return(true)
Timecop.freeze(Time.current) do
worker.perform
expect(RssReader).not_to have_received(:get_all_articles)
expect(Feeds::ImportArticlesWorker).to have_received(:perform_async).with(4.hours.ago)
end
end
it "short circuits if it's running on DEV" do
allow(SiteConfig).to receive(:dev_to?).and_return(true)
allow(RssReader).to receive(:get_all_articles)
allow(FeatureFlag).to receive(:enabled?)
worker.perform
expect(RssReader).not_to have_received(:get_all_articles)
expect(FeatureFlag).not_to have_received(:enabled?)
end
end
end

39
spec/workers/feeds/import_articles_worker_spec.rb
View file

@ -6,35 +6,34 @@ RSpec.describe Feeds::ImportArticlesWorker, type: :worker do
include_examples "#enqueues_on_correct_queue", "medium_priority"
describe "#perform" do
it "calls the Feeds::Import to get all articles" do
it "calls the Feeds::Import defaulting to 4 hours ago" do
allow(Feeds::Import).to receive(:call)
worker.perform(1.hour.ago)
Timecop.freeze(Time.current) do
worker.perform
expect(Feeds::Import).to have_received(:call)
end
context "with user ids" do
it "calls Feeds::Import with the correct users if given user ids" do
user = create(:user)
allow(Feeds::Import).to receive(:call)
worker.perform(nil, [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: nil, earlier_than: 4.hours.ago)
end
end
context "with earlier_than time" do
it "calls Feeds::Import with the correct time if given" do
allow(Feeds::Import).to receive(:call)
it "calls the Feeds::Import with the given time" do
allow(Feeds::Import).to receive(:call)
Timecop.freeze(Time.current) do
worker.perform(4.hours.ago)
Timecop.freeze(Time.current) do
worker.perform(1.minute.ago)
expect(Feeds::Import).to have_received(:call).with(users: nil, earlier_than: 4.hours.ago)
end
expect(Feeds::Import).to have_received(:call).with(users: nil, earlier_than: 1.minute.ago)
end
end
it "calls Feeds::Import with the users from the given user ids and no time" do
user = create(:user)
allow(Feeds::Import).to receive(:call)
worker.perform(nil, [user.id])
expect(Feeds::Import).to have_received(:call).with(users: User.where(id: [user.id]), earlier_than: nil)
end
end
end

40
spec/workers/rss_reader_fetch_user_worker_spec.rb
View file

@ -1,40 +0,0 @@
require "rails_helper"
RSpec.describe RssReaderFetchUserWorker, type: :worker do
let(:worker) { subject }
# passing in a random user_id argument since the worker itself won't be executed
include_examples "#enqueues_on_correct_queue", "medium_priority", [456]
describe "#perform_now" do
let(:rss_reader_service) { instance_double(RssReader) }
before do
allow(RssReader).to receive(:new).and_return(rss_reader_service)
allow(rss_reader_service).to receive(:fetch_user)
end
context "when user found and feed_url present" do
let(:user) { double }
before do
allow(User).to receive(:find_by).and_return(user)
allow(user).to receive(:feed_url).and_return(:feed_url)
allow(user).to receive(:id)
end
it "calls the service" do
worker.perform(user.id)
expect(rss_reader_service).to have_received(:fetch_user).with(user).once
end
end
context "when no user found" do
it "does not call the service" do
allow(User).to receive(:find_by)
worker.perform(9999)
expect(rss_reader_service).not_to have_received(:fetch_user)
end
end
end
end