diff --git a/app/jobs/podcasts/get_episodes_job.rb b/app/jobs/podcasts/get_episodes_job.rb index cdf3bb6c8..699e308eb 100644 --- a/app/jobs/podcasts/get_episodes_job.rb +++ b/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 diff --git a/app/models/podcast.rb b/app/models/podcast.rb index fe72cda31..ecfe3ecf1 100644 --- a/app/models/podcast.rb +++ b/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 diff --git a/app/models/podcast_episode.rb b/app/models/podcast_episode.rb index 976827274..a4159e065 100644 --- a/app/models/podcast_episode.rb +++ b/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 diff --git a/app/services/podcasts/feed.rb b/app/services/podcasts/feed.rb index b0a77e5f0..a00656aa5 100644 --- a/app/services/podcasts/feed.rb +++ b/app/services/podcasts/feed.rb @@ -3,11 +3,15 @@ require "rss/itunes" 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 @@ module Podcasts 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 diff --git a/db/migrate/20190723094834_add_index_to_podcasts_episodes.rb b/db/migrate/20190723094834_add_index_to_podcasts_episodes.rb new file mode 100644 index 000000000..523e3daca --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 0115224b6..d00268697 100644 --- a/db/schema.rb +++ b/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 @@ ActiveRecord::Schema.define(version: 2019_07_11_070019) do 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| diff --git a/spec/jobs/podcasts/get_episodes_job_spec.rb b/spec/jobs/podcasts/get_episodes_job_spec.rb index bea738964..b2eed9616 100644 --- a/spec/jobs/podcasts/get_episodes_job_spec.rb +++ b/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 diff --git a/spec/jobs/shared_examples/enqueues_job.rb b/spec/jobs/shared_examples/enqueues_job.rb index 8607952a6..f0ee70486 100644 --- a/spec/jobs/shared_examples/enqueues_job.rb +++ b/spec/jobs/shared_examples/enqueues_job.rb @@ -2,8 +2,8 @@ RSpec.shared_examples "#enqueues_job" do |queue_name, args| 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 diff --git a/spec/models/podcast_spec.rb b/spec/models/podcast_spec.rb index 4755d8229..63052090e 100644 --- a/spec/models/podcast_spec.rb +++ b/spec/models/podcast_spec.rb @@ -65,6 +65,27 @@ RSpec.describe Podcast, type: :model do 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) { "http://podcast.example/file.mp3" } diff --git a/spec/requests/podcast_episodes_api_spec.rb b/spec/requests/podcast_episodes_api_spec.rb index 29196aa6c..d61884072 100644 --- a/spec/requests/podcast_episodes_api_spec.rb +++ b/spec/requests/podcast_episodes_api_spec.rb @@ -13,7 +13,7 @@ RSpec.describe "ArticlesApi", type: :request, vcr: vcr_option do 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 diff --git a/spec/requests/stories_index_spec.rb b/spec/requests/stories_index_spec.rb index 6107a62d1..dbdc78b42 100644 --- a/spec/requests/stories_index_spec.rb +++ b/spec/requests/stories_index_spec.rb @@ -80,6 +80,7 @@ RSpec.describe "StoriesIndex", type: :request do 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 diff --git a/spec/services/podcasts/feed_spec.rb b/spec/services/podcasts/feed_spec.rb index b6f9d01c6..80bd38071 100644 --- a/spec/services/podcasts/feed_spec.rb +++ b/spec/services/podcasts/feed_spec.rb @@ -19,7 +19,7 @@ RSpec.describe Podcasts::Feed, vcr: vcr_option do 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") @@ -27,11 +27,42 @@ RSpec.describe Podcasts::Feed, vcr: vcr_option do 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 @@ RSpec.describe Podcasts::Feed, vcr: vcr_option do 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 @@ RSpec.describe Podcasts::Feed, vcr: vcr_option do 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 @@ RSpec.describe Podcasts::Feed, vcr: vcr_option do 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 diff --git a/spec/system/podcasts/user_visits_podcasts_root_page_spec.rb b/spec/system/podcasts/user_visits_podcasts_root_page_spec.rb index 1bb4704d6..7ba9009a7 100644 --- a/spec/system/podcasts/user_visits_podcasts_root_page_spec.rb +++ b/spec/system/podcasts/user_visits_podcasts_root_page_spec.rb @@ -4,7 +4,7 @@ RSpec.describe "User visits /pod page", type: :system do 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" }