Add missing features to new feed importer (#11501)

* Add explanatory comment to how RssReaderFetchUserWorker works

* Add Feeds::ImportWorker worker

* Enable Feeds::Import for single user feed fetching behind a feature flag

* Add Feeds::ValidateUrl service

* Add feature flag to user's validate feed URL feature

* Remove todo notices

* Add feature flag to fetch_all_rss task

* Add feature flag to RssReaderWorker

* Bring back short-circuit for Articles::RssReaderWorker

app/controllers/users_controller.rb

@@ -40,7 +40,10 @@ class UsersController < ApplicationController
     set_current_tab(params["user"]["tab"])
 
     if @user.update(permitted_attributes(@user))
-      RssReaderFetchUserWorker.perform_async(@user.id) if @user.feed_url.present?
+      # NOTE: [@rhymes] this queues a job to fetch the feed each time the profile is updated, regardless if the user
+      # explicitly requested "Feed fetch now" or simply updated any other field
+      import_articles_from_feed(@user)
+
       notice = "Your profile was successfully updated."
       if config_changed?
         notice = "Your config has been updated. Refresh to see all changes."
@@ -325,4 +328,12 @@ class UsersController < ApplicationController
   def destroy_request_in_progress?
     Rails.cache.exist?("user-destroy-token-#{@user.id}")
   end
+
+  def import_articles_from_feed(user)
+    return if user.feed_url.blank?
+
+    worker = FeatureFlag.enabled?(:feeds_import) ? Feeds::ImportArticlesWorker : RssReaderFetchUserWorker
+
+    worker.perform_async(user.id)
+  end
 end

app/models/user.rb

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

app/services/feeds/import.rb

@@ -1,6 +1,3 @@
-# TODO: [rhymes]
-# => add Feeds::ImportUser to fetch a single user
-# => add Feeds::ValidateFeedUrl to validate a single feed URL
 module Feeds
   class Import
     def self.call(users: nil)

app/services/feeds/validate_url.rb

@@ -0,0 +1,26 @@
+module Feeds
+  class ValidateUrl
+    def self.call(feed_url)
+      new(feed_url).call
+    end
+
+    def initialize(feed_url)
+      @feed_url = feed_url
+    end
+
+    def call
+      return false if feed_url.blank?
+
+      xml = HTTParty.get(feed_url, timeout: 10).body
+      Feedjira.parse(xml)
+
+      true
+    rescue Feedjira::NoParserAvailable
+      false
+    end
+
+    private
+
+    attr_reader :feed_url
+  end
+end

app/workers/articles/dev_feeds_import_worker.rb

@@ -1,23 +0,0 @@
-# NOTE: [rhymes]
-# This script will soon be removed. We need it to collect monitoring data on
-# Datadog about the production behavior of the `Feeds::Import` class
-module Articles
-  class DevFeedsImportWorker
-    include Sidekiq::Worker
-
-    sidekiq_options queue: :medium_priority, retry: 10
-
-    def perform(user_ids = [])
-      return unless SiteConfig.community_name == "DEV"
-      return if Rails.cache.read("cancel_feeds_import").present?
-
-      users = if user_ids.present?
-                User.where(id: user_ids)
-              else
-                User.with_feed
-              end
-
-      ::Feeds::Import.call(users: users)
-    end
-  end
-end

app/workers/articles/dev_rss_reader_worker.rb

@@ -1,18 +0,0 @@
-# NOTE: [rhymes]
-# This script will soon be removed. We need it to collect monitoring data on
-# Datadog about the production behavior of the `RssReader` class
-module Articles
-  class DevRssReaderWorker
-    include Sidekiq::Worker
-
-    sidekiq_options queue: :medium_priority, retry: 10
-
-    def perform
-      return unless SiteConfig.community_name == "DEV"
-      return if Rails.cache.read("cancel_rss_job").present?
-
-      # we force fetch to have realistic data
-      RssReader.get_all_articles(force: true)
-    end
-  end
-end

app/workers/articles/rss_reader_worker.rb

@@ -10,8 +10,12 @@ module Articles
       # for our large DEV community. Smaller Forems should be able to handle it no problem
       return if SiteConfig.community_name == "DEV"
 
-      # don't force fetch. Fetch "random" subset instead of all of them.
-      RssReader.get_all_articles(force: false)
+      if FeatureFlag.enabled?(:feeds_import)
+        ::Feeds::ImportArticlesWorker.perform_async
+      else
+        # don't force fetch. Fetch "random" subset instead of all of them.
+        ::RssReader.get_all_articles(force: false)
+      end
     end
   end
 end

app/workers/feeds/import_articles_worker.rb

@@ -0,0 +1,19 @@
+module Feeds
+  class ImportArticlesWorker
+    include Sidekiq::Worker
+
+    sidekiq_options queue: :medium_priority, retry: 10
+
+    def perform(user_ids = [])
+      user_ids = Array.wrap(user_ids)
+
+      users = if user_ids.present?
+                User.where(id: user_ids)
+              else
+                User.with_feed
+              end
+
+      ::Feeds::Import.call(users: users)
+    end
+  end
+end

lib/tasks/fetch.rake

@@ -1,9 +1,12 @@
 desc "This task is called by the Heroku scheduler add-on"
 
 task fetch_all_rss: :environment do
-  Rails.application.eager_load!
-
-  RssReader.get_all_articles(force: false) # don't force fetch. Fetch "random" subset instead of all of them.
+  if FeatureFlag.enabled?(:feeds_import)
+    Feeds::Import.call
+  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

spec/models/user_spec.rb

@@ -267,6 +267,50 @@ RSpec.describe User, type: :model do
       expect(user.errors[:base].to_s).to include("could not be saved. Rate limit reached")
       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
+
+      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
   end
 
   describe "#after_commit" do

spec/requests/user/user_settings_spec.rb

@@ -254,6 +254,32 @@ RSpec.describe "UserSettings", type: :request do
         end
       end
     end
+
+    context "when requesting a fetch of the feed", vcr: { cassette_name: "feeds_import_medium_vaidehi" } do
+      let(:feed_url) { "https://medium.com/feed/@vaidehijoshi" }
+      let(:user) { create(:user, feed_url: feed_url) }
+
+      it "invokes RssReaderFetchUserWorker" do
+        allow(Feeds::ImportArticlesWorker).to receive(:perform_async).with(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(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(user.id)
+        expect(RssReaderFetchUserWorker).not_to have_received(:perform_async).with(user.id)
+      end
+    end
   end
 
   describe "POST /users/update_language_settings" do

spec/services/feeds/import_spec.rb

@@ -26,7 +26,7 @@ RSpec.describe Feeds::Import, type: :service, vcr: true, db_strategy: :truncatio
       expect { described_class.call }.not_to change(Article, :count)
     end
 
-    it "parses correctly", vcr: { cassette_name: "rss_reader_fetch_articles" } do
+    it "parses correctly", vcr: { cassette_name: "feeds_import" } do
       described_class.call
 
       verify format: :txt do
@@ -128,29 +128,29 @@ RSpec.describe Feeds::Import, type: :service, vcr: true, db_strategy: :truncatio
     end
   end
 
-  # describe "feeds parsing and regressions" do
-  #   it "parses https://medium.com/feed/@dvirsegal correctly", vcr: { cassette_name: "feeds_import_dvirsegal" } do
-  #     user = create(:user, feed_url: "https://medium.com/feed/@dvirsegal")
+  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
+      expect do
+        described_class.call(users: User.where(id: user.id))
+      end.to change(user.articles, :count).by(10)
+    end
 
-  #   it "converts/replaces <picture> tags to <img>", vcr: { cassette_name: "feeds_import_swimburger" } do
-  #     user = create(:user, feed_url: "https://swimburger.net/atom.xml")
+    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)
+      expect do
+        described_class.call(users: User.where(id: user.id))
+      end.to change(user.articles, :count).by(10)
 
-  #     body_markdown = user.articles.last.body_markdown
+      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).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
+      expect(body_markdown).to include(expected_image_markdown)
+    end
+  end
 end

spec/services/feeds/validate_url_spec.rb

@@ -0,0 +1,18 @@
+require "rails_helper"
+
+RSpec.describe Feeds::ValidateUrl, type: :service, vcr: true do
+  let(:invalid_feed_url) { "https://example.com" }
+  let(:valid_feed_url) { "https://medium.com/feed/@vaidehijoshi" }
+
+  it "returns false for empty URL" do
+    expect(described_class.call("")).to be(false)
+  end
+
+  it "returns false for an invalid feed URL", vcr: { cassette_name: "feeds_validate_url_invalid" } do
+    expect(described_class.call(invalid_feed_url)).to be(false)
+  end
+
+  it "returns true for an valid feed URL", vcr: { cassette_name: "feeds_import_medium_vaidehi" } do
+    expect(described_class.call(valid_feed_url)).to be(true)
+  end
+end

spec/support/fixtures/vcr_cassettes/feeds_validate_url_invalid.yml

@@ -0,0 +1,65 @@
+---
+http_interactions:
+- request:
+    method: get
+    uri: https://example.com/
+    body:
+      encoding: US-ASCII
+      string: ''
+    headers:
+      Accept-Encoding:
+      - gzip;q=1.0,deflate;q=0.6,identity;q=0.3
+      Accept:
+      - "*/*"
+      User-Agent:
+      - Ruby
+  response:
+    status:
+      code: 200
+      message: OK
+    headers:
+      Accept-Ranges:
+      - bytes
+      Age:
+      - '596646'
+      Cache-Control:
+      - max-age=604800
+      Content-Type:
+      - text/html; charset=UTF-8
+      Date:
+      - Thu, 19 Nov 2020 17:47:30 GMT
+      Etag:
+      - '"3147526947"'
+      Expires:
+      - Thu, 26 Nov 2020 17:47:30 GMT
+      Last-Modified:
+      - Thu, 17 Oct 2019 07:18:26 GMT
+      Server:
+      - ECS (dcb/7FA3)
+      Vary:
+      - Accept-Encoding
+      X-Cache:
+      - HIT
+      Content-Length:
+      - '648'
+    body:
+      encoding: ASCII-8BIT
+      string: "<!doctype html>\n<html>\n<head>\n    <title>Example Domain</title>\n\n
+        \   <meta charset=\"utf-8\" />\n    <meta http-equiv=\"Content-type\" content=\"text/html;
+        charset=utf-8\" />\n    <meta name=\"viewport\" content=\"width=device-width,
+        initial-scale=1\" />\n    <style type=\"text/css\">\n    body {\n        background-color:
+        #f0f0f2;\n        margin: 0;\n        padding: 0;\n        font-family: -apple-system,
+        system-ui, BlinkMacSystemFont, \"Segoe UI\", \"Open Sans\", \"Helvetica Neue\",
+        Helvetica, Arial, sans-serif;\n        \n    }\n    div {\n        width:
+        600px;\n        margin: 5em auto;\n        padding: 2em;\n        background-color:
+        #fdfdff;\n        border-radius: 0.5em;\n        box-shadow: 2px 3px 7px 2px
+        rgba(0,0,0,0.02);\n    }\n    a:link, a:visited {\n        color: #38488f;\n
+        \       text-decoration: none;\n    }\n    @media (max-width: 700px) {\n        div
+        {\n            margin: 0 auto;\n            width: auto;\n        }\n    }\n
+        \   </style>    \n</head>\n\n<body>\n<div>\n    <h1>Example Domain</h1>\n
+        \   <p>This domain is for use in illustrative examples in documents. You may
+        use this\n    domain in literature without prior coordination or asking for
+        permission.</p>\n    <p><a href=\"https://www.iana.org/domains/example\">More
+        information...</a></p>\n</div>\n</body>\n</html>\n"
+  recorded_at: Thu, 19 Nov 2020 17:47:30 GMT
+recorded_with: VCR 6.0.0

spec/workers/articles/dev_feeds_import_worker_spec.rb

@@ -1,54 +0,0 @@
-require "rails_helper"
-
-RSpec.describe Articles::DevFeedsImportWorker, type: :worker do
-  let(:worker) { subject }
-
-  include_examples "#enqueues_on_correct_queue", "medium_priority"
-
-  describe "#perform" do
-    it "does not call the Feeds::Import for non DEV communities" do
-      allow(SiteConfig).to receive(:community_name).and_return("NotDEV")
-      allow(Feeds::Import).to receive(:call)
-
-      worker.perform
-
-      expect(Feeds::Import).not_to have_received(:call)
-    end
-
-    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")
-
-      allow(Feeds::Import).to receive(:call)
-
-      worker.perform
-
-      expect(Feeds::Import).not_to have_received(:call)
-    end
-
-    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)
-
-      worker.perform
-
-      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

spec/workers/articles/dev_rss_reader_worker_spec.rb

@@ -1,39 +0,0 @@
-require "rails_helper"
-
-RSpec.describe Articles::DevRssReaderWorker, type: :worker do
-  let(:worker) { subject }
-
-  include_examples "#enqueues_on_correct_queue", "medium_priority"
-
-  describe "#perform" do
-    it "does not call the RssReader for non DEV communities" do
-      allow(SiteConfig).to receive(:community_name).and_return("NotDEV")
-      allow(RssReader).to receive(:get_all_articles)
-
-      worker.perform
-
-      expect(RssReader).not_to have_received(:get_all_articles)
-    end
-
-    it "does not call the RssReader 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_rss_job").and_return("true")
-
-      allow(RssReader).to receive(:get_all_articles)
-
-      worker.perform
-
-      expect(RssReader).not_to have_received(:get_all_articles)
-    end
-
-    it "calls the RssReader to get all articles" do
-      allow(SiteConfig).to receive(:community_name).and_return("DEV")
-      allow(Rails.cache).to receive(:read).with("cancel_rss_job").and_return(nil)
-      allow(RssReader).to receive(:get_all_articles)
-
-      worker.perform
-
-      expect(RssReader).to have_received(:get_all_articles).with(force: true)
-    end
-  end
-end

spec/workers/articles/rss_reader_worker_spec.rb

@@ -6,10 +6,44 @@ RSpec.describe Articles::RssReaderWorker, type: :worker do
   include_examples "#enqueues_on_correct_queue", "medium_priority"
 
   describe "#perform" do
-    it "updates RssReader articles" 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(RssReader).to receive(:get_all_articles)
+      allow(FeatureFlag).to receive(:enabled?).with(:feeds_import).and_return(true)
+
+      sidekiq_assert_enqueued_jobs(1, only: Feeds::ImportArticlesWorker) do
+        worker.perform
+      end
+
+      expect(RssReader).not_to have_received(:get_all_articles)
+    end
+
+    it "short circuits if it's running on DEV" do
+      allow(SiteConfig).to receive(:community_name).and_return("DEV")
+      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

spec/workers/feeds/import_articles_worker_spec.rb

@@ -0,0 +1,28 @@
+require "rails_helper"
+
+RSpec.describe Feeds::ImportArticlesWorker, type: :worker do
+  let(:worker) { subject }
+
+  include_examples "#enqueues_on_correct_queue", "medium_priority"
+
+  describe "#perform" do
+    it "calls the Feeds::Import to get all articles" do
+      allow(Feeds::Import).to receive(:call)
+
+      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([user.id])
+
+        expect(Feeds::Import).to have_received(:call).with(users: User.where(id: [user.id]))
+      end
+    end
+  end
+end