-
Notifications
You must be signed in to change notification settings - Fork 0
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
API episode feeds #1027
Conversation
end | ||
|
||
# NOTE: this DOES NOT apply the display_episodes_count | ||
scope :in_default_feed, -> { joins(:feeds).where(feeds: {slug: nil}).published } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
@@ -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/ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few questions - like do we need a public /episodes endpoint not limited a podcast/feed, should we allow offset/windowing the default feed?
@@ -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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very clean, makes sense
end | ||
|
||
# NOTE: this DOES NOT apply the display_episodes_count | ||
scope :in_default_feed, -> { joins(:feeds).where(feeds: {slug: nil}).published } |
There was a problem hiding this comment.
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?
@@ -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/ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm persuaded. lgtm
After #1019.
Closes https://github.com/PRX/internal/issues/1104. Should QA that castle API scraping still works. But I think it will.
default
, and others are their slugs:Fallout: apply display_episodes_count to API results.