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

Override enclosure url and/or prefix #1163

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
2 changes: 2 additions & 0 deletions app/controllers/episode_media_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ def episode_params
:lock_version,
:medium,
:ad_breaks,
:enclosure_override_url,
:enclosure_override_prefix,
contents_attributes: %i[id position original_url file_size _destroy _retry],
uncut_attributes: %i[id segmentation original_url file_size _destroy _retry]
)
Expand Down
4 changes: 2 additions & 2 deletions app/helpers/episodes_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,9 @@ def episode_status_class(episode)
def episode_media_status(episode)
if episode_all_media(episode).any? { |m| upload_problem?(m) }
"error"
elsif episode_all_media(episode).any? { |m| upload_processing?(m) }
elsif episode_all_media(episode).any? { |m| upload_processing?(m) } || episode.override_processing?
"processing"
elsif episode.media_ready?(true)
elsif episode.media_ready?(true) || episode.override_ready?
"complete"
elsif episode.published_at.present?
"incomplete-published"
Expand Down
136 changes: 130 additions & 6 deletions app/models/concerns/episode_media.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,99 @@ module EpisodeMedia
extend ActiveSupport::Concern

included do
enum :medium, [:audio, :uncut, :video, :override], prefix: true

# 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
Copy link
Member

Choose a reason for hiding this comment

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

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!

has_many :contents, -> { order("position ASC, created_at DESC") }, autosave: true, dependent: :destroy, inverse_of: :episode
has_one :uncut, -> { order("created_at DESC") }, autosave: true, dependent: :destroy, inverse_of: :episode
has_one :external_media_resource, -> { order("created_at DESC") }, autosave: true, dependent: :destroy, inverse_of: :episode

accepts_nested_attributes_for :contents, allow_destroy: true, reject_if: ->(c) { c[:id].blank? && c[:original_url].blank? }
accepts_nested_attributes_for :uncut, allow_destroy: true, reject_if: ->(u) { u[:id].blank? && u[:original_url].blank? }

validate :validate_media_ready, if: :strict_validations

after_save :destroy_out_of_range_contents, if: ->(e) { e.segment_count_previously_changed? }
after_save :analyze_external_media
end

def validate_media_ready
return unless published_at.present? && media?

# media must be complete on _initial_ publish
# otherwise - having files in any status is good enough
is_ready =
if published_at_was.blank?
media_ready?(true) || override_ready?
Copy link
Member

Choose a reason for hiding this comment

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

Why make a separate method, instead of just including the override logic in def media_ready??

Copy link
Member

Choose a reason for hiding this comment

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

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?
Copy link
Member

Choose a reason for hiding this comment

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

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).

else
media_ready?(false)
end

unless is_ready
errors.add(:base, :media_not_ready, message: "media not ready")
end
end

def medium=(new_medium)
super

if medium_changed? && medium_was.present?
if medium_was == "uncut" && medium == "audio"
uncut&.mark_for_destruction
elsif medium_was == "audio" && medium == "uncut"
if (c = contents.first)
build_uncut.tap do |u|
u.file_size = contents.first.file_size
u.duration = contents.first.duration

# use the feeder cdn url for older completed files
is_old = (Time.now - c.created_at) > 24.hours
u.original_url = (c.status_complete? && is_old) ? c.url : c.original_url
end
end
contents.each(&:mark_for_destruction)
else
contents.each(&:mark_for_destruction)
end
end

self.segment_count = 1 if medium_video? || medium_override?
end

def copy_media(force = false)
contents.each { |c| c.copy_media(force) }
images.each { |i| i.copy_media(force) }
transcript&.copy_media(force)
uncut&.copy_media(force)
end

def segment_range
1..segment_count.to_i
end

def build_contents
segment_range.map do |p|
contents.find { |c| c.position == p } || contents.build(position: p)
end
end

def destroy_out_of_range_contents
if segment_count.present? && segment_count.positive?
contents.where.not(position: segment_range.to_a).destroy_all
end
end

def feed_ready?
!media? || complete_media?
!media? || complete_media? || override_ready?
Copy link
Member

Choose a reason for hiding this comment

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

Feels like def complete_media? should also be checking the override?

end

def cut_media_version!
Expand Down Expand Up @@ -97,7 +182,9 @@ def media_content_type(feed = nil)
feed_content_type = feed.try(:mime_type)

# if audio AND feed has a mime type, dovetail will transcode to that
if (media_content_type || "").starts_with?("audio")
if override?
external_media_resource&.mime_type || "audio/mpeg"
elsif (media_content_type || "").starts_with?("audio")
feed_content_type || media_content_type
elsif media_content_type
media_content_type
Expand All @@ -111,11 +198,19 @@ def video_content_type?(*args)
end

def media_duration
media.inject(0.0) { |s, c| s + c.duration.to_f } + podcast.try(:duration_padding).to_f
if override?
external_media_resource&.duration
else
media.inject(0.0) { |s, c| s + c.duration.to_f } + podcast.try(:duration_padding).to_f
end
end

def media_file_size
media.inject(0) { |s, c| s + c.file_size.to_i }
if override?
external_media_resource&.file_size
else
media.inject(0) { |s, c| s + c.file_size.to_i }
end
end

def media_ready?(must_be_complete = true)
Expand All @@ -131,19 +226,48 @@ def media_ready?(must_be_complete = true)
end

def media_status
states = media.map(&:status).uniq
states = if override?
[external_media_resource&.status]
else
media.map(&:status).uniq
end
if !(%w[started created processing retrying] & states).empty?
"processing"
elsif states.any? { |s| s == "error" }
"error"
elsif states.any? { |s| s == "invalid" }
"invalid"
elsif media_ready?
elsif media_ready? || override_ready?
"complete"
end
end

def media_url
media.first.try(:href)
end

def override_ready?
override? && external_media_ready?
end

def override_processing?
override? && !external_media_ready?
end

def override?
medium_override? || !enclosure_override_url.blank?
end

def external_media_ready?
external_media_resource&.status_complete?
end

def analyze_external_media
Copy link
Member

Choose a reason for hiding this comment

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

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.

if enclosure_override_url.blank?
external_media_resource&.destroy
elsif enclosure_override_url != external_media_resource&.original_url
external_media_resource&.destroy
create_external_media_resource(original_url: enclosure_override_url)
end
end
end
13 changes: 11 additions & 2 deletions app/models/enclosure_url_builder.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,17 @@ def podcast_episode_url(podcast, episode, feed = nil)
feed ||= podcast.default_feed
prefix = feed.try(:enclosure_prefix)

url = base_enclosure_url(podcast, episode, feed)
enclosure_prefix_url(url, prefix)
base_url = if !episode.enclosure_override_url.blank?
episode.enclosure_override_url
else
base_enclosure_url(podcast, episode, feed)
end

if episode.enclosure_override_prefix
base_url
else
enclosure_prefix_url(base_url, prefix)
end
end

def base_enclosure_url(podcast, episode, feed)
Expand Down
83 changes: 0 additions & 83 deletions app/models/episode.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,22 +23,15 @@ class Episode < ApplicationRecord

acts_as_paranoid

serialize :overrides, coder: HashSerializer
Copy link
Member

Choose a reason for hiding this comment

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

Did this just go away?


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

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
has_many :images, -> { order("created_at DESC") }, class_name: "EpisodeImage", autosave: true, dependent: :destroy, inverse_of: :episode

has_one :ready_image, -> { complete_or_replaced.order("created_at DESC") }, class_name: "EpisodeImage"
has_one :uncut, -> { order("created_at DESC") }, autosave: true, dependent: :destroy, inverse_of: :episode
has_one :transcript, -> { order("created_at DESC") }, dependent: :destroy, inverse_of: :episode

accepts_nested_attributes_for :contents, allow_destroy: true, reject_if: ->(c) { c[:id].blank? && c[:original_url].blank? }
accepts_nested_attributes_for :images, allow_destroy: true, reject_if: ->(i) { i[:id].blank? && i[:original_url].blank? }
accepts_nested_attributes_for :uncut, allow_destroy: true, reject_if: ->(u) { u[:id].blank? && u[:original_url].blank? }
accepts_nested_attributes_for :transcript, allow_destroy: true, reject_if: ->(t) { t[:id].blank? && t[:original_url].blank? }

validates :podcast_id, :guid, presence: true
Expand All @@ -53,12 +46,10 @@ class Episode < ApplicationRecord
validates :explicit, inclusion: {in: %w[true false]}, allow_nil: true
validates :segment_count, presence: true, if: :strict_validations
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

before_validation :set_defaults, :set_external_keyword, :sanitize_text

after_save :publish_updated, if: ->(e) { e.published_at_previously_changed? }
after_save :destroy_out_of_range_contents, if: ->(e) { e.segment_count_previously_changed? }

scope :published, -> { where("episodes.published_at IS NOT NULL AND episodes.published_at <= now()") }
scope :published_by, ->(offset) { where("episodes.published_at IS NOT NULL AND episodes.published_at <= ?", Time.now - offset) }
Expand All @@ -71,8 +62,6 @@ class Episode < ApplicationRecord
scope :dropdate_asc, -> { reorder(Arel.sql("#{DROP_DATE} ASC NULLS FIRST")) }
scope :dropdate_desc, -> { reorder(Arel.sql("#{DROP_DATE} DESC NULLS LAST")) }

enum :medium, [:audio, :uncut, :video], prefix: true

alias_attribute :number, :episode_number
alias_attribute :season, :season_number

Expand Down Expand Up @@ -198,36 +187,6 @@ def url_was
super || embed_player_landing_url(podcast, self)
end

def medium=(new_medium)
super

if medium_changed? && medium_was.present?
if medium_was == "uncut" && medium == "audio"
uncut&.mark_for_destruction
elsif medium_was == "audio" && medium == "uncut"
if (c = contents.first)
build_uncut.tap do |u|
u.file_size = contents.first.file_size
u.duration = contents.first.duration

# use the feeder cdn url for older completed files
is_old = (Time.now - c.created_at) > 24.hours
u.original_url = (c.status_complete? && is_old) ? c.url : c.original_url
end
end
contents.each(&:mark_for_destruction)
else
contents.each(&:mark_for_destruction)
end
end

self.segment_count = 1 if medium_video?
end

def overrides
self[:overrides] ||= HashWithIndifferentAccess.new
end

def categories
self[:categories] || []
end
Expand All @@ -236,13 +195,6 @@ def categories=(cats)
self[:categories] = sanitize_categories(cats, false).presence
end

def copy_media(force = false)
contents.each { |c| c.copy_media(force) }
images.each { |i| i.copy_media(force) }
transcript&.copy_media(force)
uncut&.copy_media(force)
end

def publish!
Rails.logger.tagged("Episode#publish!") do
apple_mark_for_reupload!
Expand Down Expand Up @@ -302,22 +254,6 @@ def feeder_cdn_host
ENV["FEEDER_CDN_HOST"]
end

def segment_range
1..segment_count.to_i
end

def build_contents
segment_range.map do |p|
contents.find { |c| c.position == p } || contents.build(position: p)
end
end

def destroy_out_of_range_contents
if segment_count.present? && segment_count.positive?
contents.where.not(position: segment_range.to_a).destroy_all
end
end

def published_or_released_date
if published_at.present?
published_at
Expand All @@ -326,25 +262,6 @@ def published_or_released_date
end
end

def validate_media_ready
return unless published_at.present? && media?

# media must be complete on _initial_ publish
# otherwise - having files in any status is good enough
is_ready =
if published_at_was.blank?
media_ready?(true)
elsif medium_uncut?
uncut.present? && !uncut.marked_for_destruction?
else
media_ready?(false)
end

unless is_ready
errors.add(:base, :media_not_ready, message: "media not ready")
end
end

def ready_transcript
transcript if transcript&.status_complete?
end
Expand Down
Loading
Loading