Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API episode feeds #1027

Merged
merged 7 commits into from
Jun 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/controllers/api/episodes_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def decorate_query(res)
end

def list_scoped(res)
res.published
res.in_default_feed
end

def show
Expand All @@ -63,7 +63,7 @@ def visible?
respond_with_error(HalApi::Errors::NotFound.new)
elsif show_resource.deleted?
respond_with_error(ResourceGone.new)
elsif !show_resource.published?
elsif !show_resource.in_default_feed?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very clean, makes sense

if EpisodePolicy.new(pundit_user, show_resource).update?
redirect_to api_authorization_episode_path(api_version, show_resource.guid)
else
Expand Down
51 changes: 51 additions & 0 deletions app/models/concerns/episode_has_feeds.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
require "active_support/concern"

module EpisodeHasFeeds
extend ActiveSupport::Concern

included do
has_many :episodes_feeds, dependent: :delete_all
has_many :feeds, through: :episodes_feeds

after_initialize :set_default_feeds, if: :new_record?
before_validation :set_default_feeds, if: :new_record?

# TODO: this doesn't filter by display_episodes_count
scope :in_feed, ->(feed) do
Episode.joins(:episodes_feeds)
.where(episodes_feeds: {feed: feed})
.published_by(feed.episode_offset_seconds.to_i)
end

# TODO: this doesn't filter by display_episodes_count
scope :in_default_feed, -> { joins(:feeds).where(feeds: {slug: nil}).published }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not trivial to apply display_episodes_count to the /api/v1/episodes endpoint. Since it includes multiple podcasts in that single paged response. It would need to be something like:

SELECT e.id, e.published_at
FROM (
  SELECT id, podcast_id, published_at, 
    ROW_NUMBER() OVER (
      PARTITION BY podcast_id ORDER BY published_at DESC
    ) AS row
  FROM episodes
  WHERE published_at <= NOW()
) e 
INNER JOIN feeds f ON (f.slug IS NULL AND f.podcast_id = e.podcast_id)
WHERE (display_episodes_count IS NULL OR row <= display_episodes_count)

That isn't exactly quick, when returning all podcasts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, yeah, I see your point; I think that's probably fine not to worry about - it makes me wonder if we should get rid of the public /episodes endpoint - does anything/one use it? spooler should be authenticated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that's the next thing we should decide after this PR. What's the utility of our public endpoints? Just to provide a JSON version of what's in public RSS? Or something else?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say to provide a json version of the public feed and individual public episodes, and I am fairly convinced it should mirror the public feed in the scope of what data it provides, but as a JSON API, and this gets us much much closer to that

end

def set_default_feeds
if feeds.blank?
self.feeds = (podcast&.feeds || []).filter_map do |feed|
feed if feed.default? || feed.apple?
end
end
end

# TODO: this doesn't filter by display_episodes_count
def in_feed?(feed)
published_by?(feed.episode_offset_seconds.to_i) && episodes_feeds.where(feed: feed).any?
end

# TODO: this doesn't filter by display_episodes_count
def in_default_feed?
published? && feeds.where(slug: nil).any?
end

def feed_slugs
feeds.pluck(:slug).map { |s| s || "default" }
end

def feed_slugs=(slugs)
self.feeds = (podcast&.feeds || []).filter_map do |feed|
feed if slugs.try(:include?, feed.slug || "default")
end
end
end
17 changes: 5 additions & 12 deletions app/models/episode.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
class Episode < ApplicationRecord
include EpisodeAdBreaks
include EpisodeFilters
include EpisodeHasFeeds
include EpisodeMedia
include PublishingStatus
include TextSanitizer
Expand All @@ -25,8 +26,6 @@ class Episode < ApplicationRecord

belongs_to :podcast, -> { with_deleted }, touch: true

has_many :episodes_feeds, dependent: :delete_all
has_many :feeds, through: :episodes_feeds
has_many :episode_imports
has_many :contents, -> { order("position ASC, created_at DESC") }, autosave: true, dependent: :destroy, inverse_of: :episode
has_many :media_versions, -> { order("created_at DESC") }, dependent: :destroy
Expand All @@ -53,8 +52,6 @@ class Episode < ApplicationRecord
validates :segment_count, numericality: {only_integer: true, greater_than: 0, less_than_or_equal_to: MAX_SEGMENT_COUNT}, allow_nil: true
validate :validate_media_ready, if: :strict_validations

after_initialize :set_default_feeds, if: :new_record?
before_validation :set_default_feeds, if: :new_record?
before_validation :set_defaults, :set_external_keyword, :sanitize_text

after_save :publish_updated, if: ->(e) { e.published_at_previously_changed? }
Expand Down Expand Up @@ -113,6 +110,10 @@ def published?
!published_at.nil? && published_at <= Time.now
end

def published_by?(offset)
!published_at.nil? && published_at <= Time.now - offset
end

def draft?
published_at.nil?
end
Expand Down Expand Up @@ -143,14 +144,6 @@ def image=(file)
end
end

def set_default_feeds
if feed_ids.blank?
self.feed_ids = podcast&.feeds&.filter_map do |feed|
feed.id if feed.default? || feed.apple?
end
end
end

def set_defaults
guid
self.segment_count ||= 1 if new_record? && strict_validations
Expand Down
3 changes: 2 additions & 1 deletion app/models/feed.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,12 @@ class Feed < ApplicationRecord

validates :slug, uniqueness: {scope: :podcast_id}, if: :podcast_id?
validates_format_of :slug, allow_nil: true, with: /\A[0-9a-zA-Z_-]+\z/
validates_format_of :slug, without: /\A(images|\w{8}-\w{4}-\w{4}-\w{4}-\w{12})\z/
validates_format_of :slug, without: /\A(default|images|\w{8}-\w{4}-\w{4}-\w{4}-\w{12})\z/
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I'm using "default" in that feedSlugs API - that's now not allowed to be your explicit slug/path.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lots of back and forth on this in slack - I have no strong opinion, but I am fine with owning the :default as a namespace

validates :file_name, presence: true, format: {with: /\A[0-9a-zA-Z_.-]+\z/}
validates :include_zones, placement_zones: true
validates :audio_format, audio_format: true
validates :title, presence: true, unless: :default?
validates :episode_offset_seconds, numericality: {equal_to: 0}, allow_nil: true, if: :default?
kookster marked this conversation as resolved.
Show resolved Hide resolved
validates :url, http_url: true
validates :new_feed_url, http_url: true
validates :enclosure_prefix, http_url: true
Expand Down
1 change: 1 addition & 0 deletions app/representers/api/auth/episode_representer.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
class Api::Auth::EpisodeRepresenter < Api::EpisodeRepresenter
property :deleted_at, writeable: false
property :feed_slugs

collection :media,
decorator: Api::Auth::MediaResourceRepresenter,
Expand Down
6 changes: 6 additions & 0 deletions test/controllers/api/episodes_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@
assert_response 404
end

it "should return not found resources not in the default feed" do
episode.update(feeds: [])
get(:show, params: {api_version: "v1", format: "json", id: episode.guid})
assert_response 404
end

it "should list" do
refute_nil episode.id
refute_nil episode_deleted.id
Expand Down
101 changes: 101 additions & 0 deletions test/models/concerns/episode_has_feeds_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
require "test_helper"

class EpisodeHasFeedsTest < ActiveSupport::TestCase
let(:podcast) { create(:podcast) }
let(:f1) { podcast.default_feed }
let(:f2) { create(:feed, podcast: podcast, slug: "feed2") }
let(:f3) { create(:feed, podcast: podcast, slug: "feed3") }
let(:episode) { create(:episode, podcast: podcast) }

before { assert [f1, f2, f3] }

describe ".in_feed" do
it "returns episodes in a feed" do
e2 = create(:episode, podcast: podcast, feeds: [f2])
e3 = create(:episode, podcast: podcast, feeds: [f2])
assert_equal [e2, e3], Episode.in_feed(f2).order(id: :asc)

# only includes published
e2.update(published_at: 1.hour.from_now)
assert_equal [e3], Episode.in_feed(f2).order(id: :asc)

# does apply offsets
f2.episode_offset_seconds = -3601
assert_equal [e2, e3], Episode.in_feed(f2).order(id: :asc)

# does NOT apply limits
f2.display_episodes_count = 1
assert_equal [e2, e3], Episode.in_feed(f2).order(id: :asc)
end
end

describe ".in_default_feed" do
it "returns episodes in the default feed" do
assert_equal [episode], Episode.in_default_feed

# unpublished episodes not in default feed
episode.update(published_at: 1.hour.from_now)
assert_empty Episode.in_default_feed

# or episodes in other feeds
episode.update(published_at: 1.hour.ago, feeds: [])
assert_empty Episode.in_default_feed
end
end

describe "#in_feed?" do
it "checks if an episode is in a feed" do
assert episode.in_feed?(f1)
refute episode.in_feed?(f2)
refute episode.in_feed?(f3)

episode.published_at = 1.minute.from_now
refute episode.in_feed?(f1)

f1.episode_offset_seconds = -61
assert episode.in_feed?(f1)
end
end

describe "#in_default_feed?" do
it "checks if an episode is in the default feed" do
assert episode.in_default_feed?

episode.published_at = nil
refute episode.in_default_feed?

episode.published_at = 1.minute.ago
episode.feeds = [f2]
refute episode.in_default_feed?
end
end

describe "#set_default_feeds" do
it "sets default feeds on new episodes" do
# saved episodes get default+apple feeds
f3.update(type: "Feeds::AppleSubscription")
assert_equal [f1.id, f3.id], episode.feeds.map(&:id).sort

# new episodes initialized with defaults
e2 = podcast.episodes.new
assert_equal [f1.id, f3.id], e2.feeds.map(&:id).sort

# UNLESS episode already has feeds
e2 = podcast.episodes.new(feeds: [f2])
assert_equal [f2.id], e2.feeds.map(&:id)
end
end

describe "#feed_slugs" do
it "gets and sets feeds based on their slugs" do
assert_equal [f1], episode.feeds
assert_equal ["default"], episode.feed_slugs

episode.feed_slugs = ["default", "whatev", "feed3"]
assert_equal [f1, f3], episode.feeds

episode.feed_slugs = ["feed3"]
assert_equal [f3], episode.feeds
end
end
end
5 changes: 3 additions & 2 deletions test/models/concerns/release_episodes_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ class ReleaseEpisodesTest < ActiveSupport::TestCase
let(:episode) { create(:episode, published_at: 30.minutes.from_now) }
let(:podcast) { episode.podcast }
let(:feed) { podcast.default_feed }
# let(:feed) { create(:feed, podcast: podcast, slug: "the-feed") }

describe ".to_release" do
it "returns episodes/podcasts that need release" do
Expand All @@ -26,7 +27,7 @@ class ReleaseEpisodesTest < ActiveSupport::TestCase
end

it "handles negative feed offsets" do
feed.update!(episode_offset_seconds: -300)
feed.update!(slug: "slug", episode_offset_seconds: -300)
assert_empty Episode.to_release
assert_empty Podcast.to_release

Expand All @@ -41,7 +42,7 @@ class ReleaseEpisodesTest < ActiveSupport::TestCase
end

it "handles positive feed offsets" do
feed.update!(episode_offset_seconds: 300)
feed.update!(slug: "slug", episode_offset_seconds: 300)
assert_empty Episode.to_release
assert_empty Podcast.to_release

Expand Down
19 changes: 19 additions & 0 deletions test/models/feed_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,9 @@
it "restricts some slugs already used in S3" do
assert feed1.valid?

feed1.slug = "default"
refute feed1.valid?

feed1.slug = "images"
refute feed1.valid?

Expand All @@ -187,6 +190,22 @@
feed2.title = "new feed"
assert feed2.valid?
end

it "does not allow episode offsets on default" do
feed1.episode_offset_seconds = 5
assert feed1.default?
assert feed1.invalid?

feed1.episode_offset_seconds = 0
assert feed1.valid?

feed1.episode_offset_seconds = nil
assert feed1.valid?

# non-default feeds can have offsets
feed2.episode_offset_seconds = 5
assert feed2.valid?
end
end

describe "#published_url" do
Expand Down
Loading