Don't hide podcasts if they have reachable episodes (forem#3532)

* Don't hide podcasts if they have reachable episodes
Recheck episodes' reachability after the podcast status is set to
unreachable

* Update db/schema.rb

* Fix GetEpisodesJob specs

* Fix stories spec

* Episodes are reachable even if the podcast is unreachable

* Improve the Podcast.reachable query

* Fix podcasts root page spec

* Add index concurrently

app/jobs/podcasts/get_episodes_job.rb

@@ -2,11 +2,11 @@ module Podcasts
   class GetEpisodesJob < ApplicationJob
     queue_as :podcasts_get_episodes
 
-    def perform(podcast_id:, limit: 1000, force_update: false, feed: Podcasts::Feed.new)
+    def perform(podcast_id:, limit: 1000, force_update: false, feed: Podcasts::Feed)
       podcast = Podcast.find_by(id: podcast_id)
       return unless podcast
 
-      feed.get_episodes(podcast: podcast, limit: limit, force_update: force_update)
+      feed.new(podcast).get_episodes(limit: limit, force_update: force_update)
     end
   end
 end

app/models/podcast.rb

@@ -15,7 +15,7 @@ class Podcast < ApplicationRecord
 
   after_save :bust_cache
 
-  scope :reachable, -> { where(reachable: true) }
+  scope :reachable, -> { where(id: PodcastEpisode.reachable.select(:podcast_id)) }
 
   alias_attribute :path, :slug
   alias_attribute :profile_image_url, :image_url

app/models/podcast_episode.rb

@@ -25,7 +25,7 @@ class PodcastEpisode < ApplicationRecord
 
   before_validation :prefix_all_images
 
-  scope :reachable, -> { joins(:podcast).where(reachable: true, podcasts: { reachable: true }) }
+  scope :reachable, -> { where(reachable: true) }
 
   algoliasearch per_environment: true do
     attribute :id

app/services/podcasts/feed.rb

@@ -3,11 +3,15 @@
 
 module Podcasts
   class Feed
-    def get_episodes(podcast:, limit: 100, force_update: false)
+    def initialize(podcast)
+      @podcast = podcast
+    end
+
+    def get_episodes(limit: 100, force_update: false)
       rss = HTTParty.get(podcast.feed_url).body
       feed = RSS::Parser.parse(rss, false)
 
-      set_unreachable(podcast, :unparsable) && return unless feed
+      set_unreachable(:unparsable, force_update) && return unless feed
 
       get_episode = Podcasts::GetEpisode.new(podcast)
       feed.items.first(limit).each do |item|
@@ -16,17 +20,32 @@ def get_episodes(podcast:, limit: 100, force_update: false)
       podcast.update_columns(reachable: true, status_notice: "")
       feed.items.size
     rescue Net::OpenTimeout, Errno::ECONNREFUSED => _e
-      set_unreachable(podcast, :unreachable)
+      set_unreachable(:unreachable, force_update)
     rescue OpenSSL::SSL::SSLError => _e
-      set_unreachable(podcast, :ssl_failed)
+      set_unreachable(:ssl_failed, force_update)
     rescue RSS::NotWellFormedError => _e
-      set_unreachable(podcast, :unparsable)
+      set_unreachable(:unparsable, force_update)
     end
 
     private
 
-    def set_unreachable(podcast, status = :unreachable)
+    attr_reader :podcast
+
+    def set_unreachable(status = :unreachable, force_update = false)
+      # don't recheck if the podcast was already unreachable or force update is required
+      need_refetching = podcast.reachable || force_update
       podcast.update_columns(reachable: false, status_notice: I18n.t(status, scope: "podcasts.statuses"))
+      refetch_items if need_refetching
+      true
+    end
+
+    # When setting podcast feed as unreachable, we need to re-check the episodes URLs.
+    # If the episodes URLs are still reachable, the podcast will remain on the site.
+    # If they are not, the podcast will be hidden.
+    def refetch_items
+      podcast.podcast_episodes.find_each do |episode|
+        PodcastEpisodes::UpdateMediaUrlJob.perform_later(episode.id, episode.media_url)
+      end
     end
   end
 end

db/migrate/20190723094834_add_index_to_podcasts_episodes.rb

@@ -0,0 +1,7 @@
+class AddIndexToPodcastsEpisodes < ActiveRecord::Migration[5.2]
+  disable_ddl_transaction!
+
+  def change
+    add_index :podcast_episodes, :podcast_id, algorithm: :concurrently
+  end
+end

db/schema.rb

@@ -12,7 +12,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2019_07_11_070019) do
+ActiveRecord::Schema.define(version: 2019_07_23_094834) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
 
@@ -667,6 +667,7 @@
     t.string "website_url"
     t.index ["guid"], name: "index_podcast_episodes_on_guid", unique: true
     t.index ["media_url"], name: "index_podcast_episodes_on_media_url", unique: true
+    t.index ["podcast_id"], name: "index_podcast_episodes_on_podcast_id"
   end
 
   create_table "podcasts", id: :serial, force: :cascade do |t|

spec/jobs/podcasts/get_episodes_job_spec.rb

@@ -1,23 +1,24 @@
 require "rails_helper"
 
 RSpec.describe Podcasts::GetEpisodesJob, type: :job do
-  include_examples "#enqueues_job", "podcasts_get_episodes", [1, 4]
+  include_examples "#enqueues_job", "podcasts_get_episodes", podcast_id: 10
 
   describe "#perform_now" do
-    let(:feed) { double }
     let(:podcast) { create(:podcast) }
+    let(:feed) { instance_double(Podcasts::Feed) }
 
     before do
+      allow(Podcasts::Feed).to receive(:new).and_return(feed)
       allow(feed).to receive(:get_episodes)
     end
 
     it "calls the service" do
-      described_class.perform_now(podcast_id: podcast.id, limit: 5, feed: feed, force_update: true)
-      expect(feed).to have_received(:get_episodes).with(podcast: podcast, limit: 5, force_update: true).once
+      described_class.perform_now(podcast_id: podcast.id, limit: 5, force_update: true)
+      expect(feed).to have_received(:get_episodes)
     end
 
     it "doesn't call the service when the podcast is not found" do
-      described_class.perform_now(podcast_id: Podcast.maximum(:id).to_i + 1, limit: 5, feed: feed)
+      described_class.perform_now(podcast_id: Podcast.maximum(:id).to_i + 1, limit: 5)
       expect(feed).not_to have_received(:get_episodes)
     end
   end

spec/jobs/shared_examples/enqueues_job.rb

@@ -2,8 +2,8 @@
   describe "#perform_later" do
     it "enqueues the job" do
       expect do
-        described_class.perform_later(*args)
-      end.to have_enqueued_job.with(*args).on_queue(queue_name)
+        described_class.perform_later(args)
+      end.to have_enqueued_job.with(args).on_queue(queue_name)
     end
   end
 end

spec/models/podcast_spec.rb

@@ -65,6 +65,27 @@
     end
   end
 
+  describe "#reachable" do
+    let(:podcast) { create(:podcast, reachable: false) }
+    let!(:unpodcast) { create(:podcast, reachable: false) }
+    let!(:unpodcast2) { create(:podcast, reachable: false) }
+    let!(:cool_podcast) { create(:podcast, reachable: true) }
+
+    before do
+      create(:podcast_episode, reachable: true, podcast: podcast)
+      create(:podcast_episode, reachable: false, podcast: unpodcast2)
+      create(:podcast_episode, reachable: true, podcast: cool_podcast)
+    end
+
+    it "is reachable when the feed is unreachable but the podcast has reachable podcasts" do
+      reachable_ids = Podcast.reachable.pluck(:id)
+      expect(reachable_ids).to include(podcast.id)
+      expect(reachable_ids).to include(cool_podcast.id)
+      expect(reachable_ids).not_to include(unpodcast.id)
+      expect(reachable_ids).not_to include(unpodcast2.id)
+    end
+  end
+
   describe "#existing_episode" do
     let(:podcast) { create(:podcast) }
     let(:guid) { "<guid isPermaLink=\"false\">http://podcast.example/file.mp3</guid>" }

spec/requests/podcast_episodes_api_spec.rb

@@ -13,7 +13,7 @@
     stub_request(:head, "https://traffic.libsyn.com/sedaily/IFTTT.mp3").to_return(status: 200)
 
     perform_enqueued_jobs do
-      Podcasts::Feed.new.get_episodes(podcast: podcast, limit: 2)
+      Podcasts::Feed.new(podcast).get_episodes(limit: 2)
     end
   end
 

spec/requests/stories_index_spec.rb

@@ -80,6 +80,7 @@
   describe "GET podcast index" do
     it "renders page with proper header" do
       podcast = create(:podcast)
+      create(:podcast_episode, podcast: podcast)
       get "/" + podcast.slug
       expect(response.body).to include(podcast.title)
     end

spec/services/podcasts/feed_spec.rb

@@ -19,19 +19,50 @@
 
     it "sets reachable and status" do
       stub_request(:get, "http://podcast.example.com/podcast").to_return(status: 200, body: "blah")
-      described_class.new.get_episodes(podcast: unpodcast, limit: 2)
+      described_class.new(unpodcast).get_episodes(limit: 2)
       unpodcast.reload
       expect(unpodcast.reachable).to be false
       expect(unpodcast.status_notice).to include("rss couldn't be parsed")
     end
 
     it "sets reachable" do
       allow(HTTParty).to receive(:get).with("http://podcast.example.com/podcast").and_raise(Errno::ECONNREFUSED)
-      described_class.new.get_episodes(podcast: unpodcast, limit: 2)
+      described_class.new(unpodcast).get_episodes(limit: 2)
       unpodcast.reload
       expect(unpodcast.reachable).to be false
       expect(unpodcast.status_notice).to include("is not reachable")
     end
+
+    it "schedules the update url jobs when setting as unreachable" do
+      allow(HTTParty).to receive(:get).with("http://podcast.example.com/podcast").and_raise(Errno::ECONNREFUSED)
+      create_list(:podcast_episode, 2, podcast: unpodcast)
+      expect do
+        described_class.new(unpodcast).get_episodes(limit: 2)
+      end.to have_enqueued_job(PodcastEpisodes::UpdateMediaUrlJob).exactly(2)
+    end
+
+    it "re-checks episodes urls when setting as unreachable" do
+      allow(HTTParty).to receive(:get).with("http://podcast.example.com/podcast").and_raise(Errno::ECONNREFUSED)
+      episode = create(:podcast_episode, podcast: unpodcast, reachable: true, media_url: "http://podcast.example.com/ep1.mp3")
+      allow(HTTParty).to receive(:head).with("http://podcast.example.com/ep1.mp3").and_raise(Errno::ECONNREFUSED)
+      allow(HTTParty).to receive(:head).with("https://podcast.example.com/ep1.mp3").and_raise(Errno::ECONNREFUSED)
+
+      perform_enqueued_jobs do
+        described_class.new(unpodcast).get_episodes
+      end
+
+      episode.reload
+      expect(episode.reachable).to be false
+    end
+
+    it "doesn't re-check episodes reachable if the podcast was unreachable" do
+      unpodcast.update_column(:reachable, false)
+      allow(HTTParty).to receive(:get).with("http://podcast.example.com/podcast").and_raise(Errno::ECONNREFUSED)
+      create_list(:podcast_episode, 2, podcast: unpodcast)
+      expect do
+        described_class.new(unpodcast).get_episodes(limit: 2)
+      end.not_to have_enqueued_job(PodcastEpisodes::UpdateMediaUrlJob)
+    end
   end
 
   context "when ssl certificate is not valid" do
@@ -40,7 +71,7 @@
 
     it "sets ssl_failed" do
       allow(HTTParty).to receive(:get).with("http://podcast.example.com/podcast").and_raise(OpenSSL::SSL::SSLError)
-      described_class.new.get_episodes(podcast: unpodcast, limit: 2)
+      described_class.new(unpodcast).get_episodes(limit: 2)
       unpodcast.reload
       expect(unpodcast.reachable).to be false
       expect(unpodcast.status_notice).to include("SSL certificate verify failed")
@@ -56,14 +87,14 @@
     it "fetches podcast episodes" do
       expect do
         perform_enqueued_jobs do
-          described_class.new.get_episodes(podcast: podcast, limit: 2)
+          described_class.new(podcast).get_episodes(limit: 2)
         end
       end.to change(PodcastEpisode, :count).by(2)
     end
 
     it "fetches correct podcasts" do
       perform_enqueued_jobs do
-        described_class.new.get_episodes(podcast: podcast, limit: 2)
+        described_class.new(podcast).get_episodes(limit: 2)
       end
       episodes = podcast.podcast_episodes
       expect(episodes.pluck(:title).sort).to eq(["Analyse Asia with Bernard Leong", "IFTTT Architecture with Nicky Leach"])
@@ -77,12 +108,12 @@
 
     it "does not refetch already fetched episodes" do
       expect do
-        described_class.new.get_episodes(podcast: podcast, limit: 2)
+        described_class.new(podcast).get_episodes(limit: 2)
       end.not_to change(PodcastEpisode, :count)
     end
 
     it "updates published_at for existing episodes" do
-      described_class.new.get_episodes(podcast: podcast, limit: 2)
+      described_class.new(podcast).get_episodes(limit: 2)
       episode.reload
       episode2.reload
       expect(episode.published_at).to be_truthy

spec/system/podcasts/user_visits_podcasts_root_page_spec.rb

@@ -4,7 +4,7 @@
   let!(:podcast_episode) { create(:podcast_episode) }
   let!(:podcast_episode2) { create(:podcast_episode) }
   let(:podcast) { create(:podcast, reachable: false) }
-  let!(:un_podcast_episode) { create(:podcast_episode, podcast: podcast) }
+  let!(:un_podcast_episode) { create(:podcast_episode, podcast: podcast, reachable: false) }
 
   before { visit "/pod" }