Override enclosure url and/or prefix #1163
Conversation
kookster
commented
Dec 8, 2024
•
kookster
commented
- Add db columns for enclosure url and prefix overrides
- Use overrides in building the enclosure urls for podcast episodes
- Analyze external media to get size, duration, type for RSS values
- Update the episodes UI to display and set the overrides
- Add "override only" media type
There was a problem hiding this comment.
It begins with the comments!
So I think my main asks are:
- Move overrides into the existing
media_ready?logic, to avoid all the|| override?conditionals - Reuse the existing
copy_mediastuff, and maybe we just open a ticket to rename that toprocess_mediaacross the board at some point? Or justprocess? Naming is hard.
I'm still mulling over having both new Episode fields and a new MediaResource subclass. It seems a bit redundant? But there may be a reason to keep both.
Also note to self... need to QA some of the episode publishing flows/validations, to make sure this does all the usual stuff (external must be ready on initial publish, can be replaced without being ready yet, etc). I guess one issue there is that this is a has_one, so we don't have the old override in-hand to serve until the new one is ready. Edge cases!
| # NOTE: this just-in-time creates new media versions | ||
| # TODO: convert to sql, so we don't have to load/check every episode? | ||
| # TODO: stop loading non-latest media versions | ||
| scope :feed_ready, -> { includes(media_versions: :media_resources).select { |e| e.feed_ready? } } | ||
|
|
||
| has_many :media_versions, -> { order("created_at DESC") }, dependent: :destroy |
There was a problem hiding this comment.
Been thinking about this... and I guess I'd prefer all the associations stay in the actual model? Having them scattered in concerns just makes it too hard to eyeball the structure of things.
I could be convinced though, so feel free to fight it out!
| # otherwise - having files in any status is good enough | ||
| is_ready = | ||
| if published_at_was.blank? | ||
| media_ready?(true) || override_ready? |
There was a problem hiding this comment.
Why make a separate method, instead of just including the override logic in def media_ready??
There was a problem hiding this comment.
Yeah, reading further... it feels like media_ready? is the place that should check if there's an override and if it's ready. Instead of special casing it in these places.
| elsif medium_uncut? | ||
| uncut.present? && !uncut.marked_for_destruction? | ||
| elsif override? | ||
| external_media_ready? |
There was a problem hiding this comment.
I'm not sure this makes sense... so for an already-published episode, you can't alter the external media? Because it looks like this requires it to be status=complete.
(And I think this elsif should go away, and just be wrapped into the media_ready? method).
| end | ||
|
|
||
| def feed_ready? | ||
| !media? || complete_media? | ||
| !media? || complete_media? || override_ready? |
There was a problem hiding this comment.
Feels like def complete_media? should also be checking the override?
| external_media_resource&.status_complete? | ||
| end | ||
|
|
||
| def analyze_external_media |
There was a problem hiding this comment.
Feels like this could be wrapped into the existing copy_media (which yeah, is a misnomer... really means any async process for media). That would keep things more consistent, around when async jobs get fired. Thus far, we always do that in copy_media called from usually the controller. Rather than activerecord hooks.
| @@ -23,22 +23,15 @@ class Episode < ApplicationRecord | |||
|
|
|||
| acts_as_paranoid | |||
|
|
|||
| serialize :overrides, coder: HashSerializer | |||
| def copy_media(force = false) | ||
| end | ||
|
|
||
| def analyze_media(force = false) |
There was a problem hiding this comment.
Can we keep using copy_media (even if the name is misleading). We could pretty easily rename across the board to .process_media if we wanted.
