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

Apple retry uploads #707

Merged
merged 11 commits into from
Jul 25, 2023
26 changes: 12 additions & 14 deletions app/models/apple/episode.rb
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ def self.update_audio_container_reference(api, episodes)
episode_bridge_results
end

def self.remove_audio_container_reference(api, show, episodes)
def self.remove_audio_container_reference(api, show, episodes, apple_mark_for_reupload: true)
svevang marked this conversation as resolved.
Show resolved Hide resolved
return [] if episodes.empty?

(episode_bridge_results, errs) =
Expand All @@ -128,8 +128,8 @@ def self.remove_audio_container_reference(api, show, episodes)
upsert_sync_logs(episodes, episode_bridge_results)

join_on_apple_episode_id(episodes, episode_bridge_results).each do |(ep, row)|
ep.podcast_container.podcast_delivery_files.each(&:destroy)
ep.podcast_container.podcast_deliveries.each(&:destroy)
ep.feeder_episode.apple_mark_for_reupload if apple_mark_for_reupload
Rails.logger.info("Removed audio container reference for episode", {episode_id: ep.feeder_id})
end

show.reload
Expand All @@ -139,11 +139,15 @@ def self.remove_audio_container_reference(api, show, episodes)
episode_bridge_results
end

def self.publish(api, episodes, state: "PUBLISH")
def self.publish(api, show, episodes, state: "PUBLISH")
return [] if episodes.empty?

api.bridge_remote_and_retry!("publishEpisodes",
episodes.map { |e| e.publishing_state_bridge_params(state) })

# We don't get back the full episode model in the response.
# So poll for current state
poll_episode_state(api, show, episodes)
Copy link
Member Author

Choose a reason for hiding this comment

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

Tracking a few of these: There was a remote state transition, but the local data was not updated to reflect that. So make sure the local data is up to date.

end

def self.upsert_sync_logs(episodes, results)
Expand Down Expand Up @@ -381,10 +385,8 @@ def drafting?
apple_json&.dig("attributes", "publishingState") == "DRAFTING"
end

def apple_upload_complete?
pdfs = feeder_episode.apple_podcast_delivery_files

pdfs.present? && pdfs.to_a.flatten.all?(&:apple_complete?)
def container_upload_complete?
Copy link
Member Author

Choose a reason for hiding this comment

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

@cavis I hope this clarifies. I renamed to emphasize the container upload semantics here. We need to deliver the audio file (Delivery, DeliveryFiles) and the container upload (PodcastContaner files attribute) will be complete (a file marked present).

The overall Apple::Episode#synced_with_apple? covers the rest. That method filters the episodes that go into the publishing routines in Apple::Publisher.

feeder_episode.apple_podcast_container.container_upload_satisfied?
end

def audio_asset_vendor_id
Expand Down Expand Up @@ -435,15 +437,11 @@ def reset_for_upload!
end

def synced_with_apple?
audio_asset_state_success? && apple_upload_complete? && !drafting?
audio_asset_state_success? && container_upload_complete? && !drafting?
end

def waiting_for_asset_state?
(podcast_delivery_files.length > 0 &&
podcast_delivery_files.all?(&:delivered?) &&
podcast_delivery_files.all?(&:processed?) &&
!podcast_delivery_files.all?(&:processed_errors?) &&
!audio_asset_state_finished?)
podcast_container.delivery_settled? && !audio_asset_state_finished?
Copy link
Member Author

Choose a reason for hiding this comment

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

so now only looking the episode's audio_asset_state and at the podcast container (which manages / coordinates the deliveries).

end

def apple_id
Expand Down
52 changes: 36 additions & 16 deletions app/models/apple/podcast_container.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,6 @@ def self.probe_source_file_metadata(api, episodes)
end

def self.poll_podcast_container_state(api, episodes)
episodes = episodes.filter(&:apple_persisted?)

results = get_podcast_containers_via_episodes(api, episodes)

join_on_apple_episode_id(episodes, results, left_join: true).each do |(ep, row)|
Expand Down Expand Up @@ -133,18 +131,14 @@ def self.upsert_podcast_container(episode, row)
end

def self.get_podcast_containers_via_episodes(api, episodes)
# Only query for episodes that don't have a podcast container
# The container. Assume that if we have a container record, we don't need to poll.
eps_without_container = episodes.filter { |ep| ep.apple_persisted? && ep.missing_container? }

Copy link
Member Author

Choose a reason for hiding this comment

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

Trying to move special guards and state checks into more top level concerns (like PodcastContainer#delivery_settled?)

# Fetch the podcast containers from the episodes side of the API
response =
api.bridge_remote_and_retry!("getPodcastContainers", get_podcast_containers_bridge_params(api, eps_without_container), batch_size: 1)
api.bridge_remote_and_retry!("getPodcastContainers", get_podcast_containers_bridge_params(api, episodes), batch_size: 1)

# Rather than mangling and persisting the enumerated view of the containers in the episodes,
# just re-fetch the podcast containers from the non-list podcast container endpoint
formatted_bridge_params =
join_on_apple_episode_id(eps_without_container, response).map do |(episode, row)|
join_on_apple_episode_id(episodes, response).map do |(episode, row)|
get_urls_for_episode_podcast_containers(api, row).map do |url|
get_podcast_containers_bridge_param(episode.apple_id, url)
end
Expand Down Expand Up @@ -242,9 +236,9 @@ def files
end

def has_podcast_audio?
return false if files.blank?
return false if files.empty?

files.any? do |file|
files.all? do |file|
# Retrieve the file status from the podcast container's files attribute
file["status"] == FILE_STATUS_SUCCESS && file["assetRole"] == FILE_ASSET_ROLE_PODCAST_AUDIO
end
Expand All @@ -254,13 +248,39 @@ def missing_podcast_audio?
!has_podcast_audio?
end

def delivered?
return false if podcast_delivery_files.length == 0

(podcast_delivery_files.all?(&:delivered?) &&
podcast_delivery_files.all?(&:processed?))
end

def processed_errors?
return false if podcast_delivery_files.length == 0

podcast_delivery_files.all?(&:processed_errors?)
end

def delivery_settled?
return false if podcast_delivery_files.length == 0

delivered? && !processed_errors?
end

def skip_delivery?
container_upload_satisfied?
end

def container_upload_satisfied?
# Sets us up for a retry if something prevented the audio from being
# marked as uploaded and then processed and validated. Assuming that we
# get to that point and the audio is still missing, we should be able to
# retry.
has_podcast_audio? && delivery_settled?
end

def needs_delivery?
# Handle the case where the podcast container *does* have podcast audio,
# but doesn't have any podcast deliveries / files. This is a weird edge
# case but it amounts to checking the deliveries to see if any are there.
# If there are no deliveries, then the code that polls/checks the delivery
# status will fail. So we need to create a delivery.
podcast_deliveries.empty?
!skip_delivery?
end

def reset_source_metadata!(apple_ep)
Expand Down
4 changes: 2 additions & 2 deletions app/models/apple/podcast_delivery_file.rb
Original file line number Diff line number Diff line change
Expand Up @@ -327,14 +327,14 @@ def delivered?
def processed_errors?
return false unless asset_processing_state.present?

processed_validation_failed? || processed_duplicate?
Copy link
Member Author

Choose a reason for hiding this comment

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

Uploading a duplicate file is not really an error.

Copy link
Member

Choose a reason for hiding this comment

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

+1

processed_validation_failed?
end

def processed?
# FIXME: sometimes we get a nil assetProcessingState, but the file is uploaded
return false unless asset_processing_state.present?

processed_completed? || processed_errors?
processed_completed? || processed_duplicate? || processed_errors?
end

def asset_processing_state
Expand Down
17 changes: 15 additions & 2 deletions app/models/apple/publisher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,10 @@ def wait_for_upload_processing(eps)
pdfs = eps.map(&:podcast_delivery_files).flatten

Apple::PodcastDeliveryFile.wait_for_delivery_files(api, pdfs)

# Get the latest state of the podcast containers
# which should include synced files
Apple::PodcastContainer.poll_podcast_container_state(api, eps)
Copy link
Member Author

Choose a reason for hiding this comment

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

Again, missing local state, this time in the files Apple attribute -- which gives a readout of the successfully processed/delivered files.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm hitting this in prod (w/ test creds) just now. A podcast has episodes missing their container state, and upon retry I can't push that sync process forward. That should be fixed by this and other patches in this PR.

end
end

Expand Down Expand Up @@ -242,11 +246,20 @@ def mark_delivery_files_uploaded!(eps)

def publish_drafting!(eps)
Rails.logger.tagged("##{__method__}") do
eps = eps.select { |ep| ep.drafting? && ep.apple_upload_complete? }
eps = eps.select { |ep| ep.drafting? && ep.container_upload_complete? }

res = Apple::Episode.publish(api, eps)
res = Apple::Episode.publish(api, show, eps)
Rails.logger.info("Published #{res.length} drafting episodes.")
end
end

# Not used in any of the polling or publish routines, but useful for
# debugging. This removes the audio container reference from the episode,
# but leaves the podcast container intact.
def remove_audio_container_reference(eps, apple_mark_for_reupload: true)
Rails.logger.tagged("##{__method__}") do
Apple::Episode.remove_audio_container_reference(api, show, eps, apple_mark_for_reupload: apple_mark_for_reupload)
end
end
end
end
44 changes: 44 additions & 0 deletions test/factories/apple_podcast_container_api_response_factory.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
FactoryBot.define do
factory :podcast_container_api_response, class: OpenStruct do
transient do
podcast_container_id { "podcast-delivery-id" }
vendor_id { "43434343" }
file_name { "some.mp3" }
file_type { "audio" }
file_asset_token { "some-token" }
file_status { "In Asset Repository" }
file_asset_role { "PodcastSourceAudio" }
end

skip_create

after(:build) do |response_container, evaluator|
response_container["api_response"] = {"request_metadata" => {},
"api_url" => "https://api.podcastsconnect.apple.com/v1/podcastContainers/#{evaluator.podcast_container_id}",
"api_parameters" => {},
"api_response" =>
{"ok" => true,
"err" => false,
"val" =>
{"data" =>
{"type" => "podcastContainers",
"id" => evaluator.podcast_container_id.to_s,
"attributes" =>
{"vendorId" => evaluator.vendor_id.to_s,
"episodeDetail" => nil,
"files" =>
[{"assetToken" => evaluator.file_asset_token.to_s, "fileName" => evaluator.file_name.to_s, "fileType" => evaluator.file_type.to_s, "status" => evaluator.file_status.to_s, "assetRole" => evaluator.file_asset_role.to_s, "imageAsset" => nil}]},
"relationships" =>
{"podcastDeliveries" =>
{"links" =>
{"self" => "https://api.podcastsconnect.apple.com/v1/podcastContainers/#{evaluator.podcast_container_id}/relationships/podcastDeliveries",
"related" => "https://api.podcastsconnect.apple.com/v1/podcastContainers/#{evaluator.podcast_container_id}/podcastDeliveries",
"include" => "https://api.podcastsconnect.apple.com/v1/podcastContainers/#{evaluator.podcast_container_id}?include=podcastDeliveries"}}},
"links" => {"self" => "https://api.podcastsconnect.apple.com/v1/podcastContainers/#{evaluator.podcast_container_id}"}},
"links" => {"self" => "https://api.podcastsconnect.apple.com/v1/podcastContainers/#{evaluator.podcast_container_id}"}}}}
response_container
end

initialize_with { attributes }
end
end
9 changes: 9 additions & 0 deletions test/factories/apple_podcast_container_factory.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,14 @@
episode
sequence(:vendor_id) { |n| n.to_s }
sequence(:apple_episode_id) { |n| n.to_s }

transient do
sequence(:external_id) { |n| "apple_container_id_#{n}" }
end

after(:build) do |podcast_container, evaluator|
api_response = build(:podcast_container_api_response, podcast_container_id: evaluator.external_id)
podcast_container.apple_sync_log = SyncLog.new(external_id: evaluator.external_id, feeder_type: :podcast_containers, **api_response)
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
skip_create

after(:build) do |response_container, evaluator|
response_container["external_id"] = evaluator.podcast_delivery_file_id
response_container["feeder_type"] = :podcast_delivery_files
response_container["api_response"] = {"request_metadata" => {"apple_episode_id" => evaluator.apple_episode_id, "podcast_delivery_id" => evaluator.podcast_delivery_id},
"api_url" => "https://api.podcastsconnect.apple.com/v1/podcastDeliveryFiles/#{evaluator.podcast_delivery_file_id}",
"api_parameters" => {},
Expand Down
17 changes: 16 additions & 1 deletion test/models/apple/episode_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@
end

it "should be false if there are no podcast delivery files" do
apple_episode.stub(:podcast_delivery_files, []) do
apple_episode.podcast_container.stub(:podcast_delivery_files, []) do
assert_equal false, apple_episode.waiting_for_asset_state?
end
end
Expand Down Expand Up @@ -167,4 +167,19 @@
end
end
end

describe "#publish" do
it "should call poll! at the conclusion of the episode publishing" do
mock = Minitest::Mock.new
mock.expect(:call, nil, [apple_api, apple_show, [apple_episode]])

apple_api.stub(:bridge_remote_and_retry, nil) do
Apple::Episode.stub(:poll_episode_state, mock) do
Apple::Episode.publish(apple_api, apple_show, [apple_episode])
end
end

mock.verify
end
end
end
7 changes: 7 additions & 0 deletions test/models/apple/podcast_delivery_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,13 @@ class Apple::PodcastDeliveryTest < ActiveSupport::TestCase
episode: podcast_container.episode)
}

before do
pdf_resp_container = build(:podcast_delivery_file_api_response)
pdf = Apple::PodcastDeliveryFile.create!(podcast_delivery: podcast_delivery, episode: podcast_container.episode)
pdf.create_apple_sync_log!(**pdf_resp_container)
podcast_container.reload
end

it "should soft delete the delivery" do
assert podcast_container.persisted?
assert_equal [podcast_delivery], podcast_container.podcast_deliveries
Expand Down
27 changes: 27 additions & 0 deletions test/models/apple/publisher_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -109,4 +109,31 @@
end
end
end

describe "#publish_drafting!" do
it "should call the episode publish drafting class method" do
ep = OpenStruct.new(drafting?: true, container_upload_complete?: true)
mock = Minitest::Mock.new
mock.expect(:call, [], [apple_publisher.api, apple_publisher.show, [ep]])

Apple::Episode.stub(:publish, mock) do
apple_publisher.publish_drafting!([ep])
end

mock.verify
end
end

describe "#wait_for_upload_processing" do
it "should poll the podcast container state" do
mock = Minitest::Mock.new
mock.expect(:call, [], [apple_publisher.api, []])

Apple::PodcastContainer.stub(:poll_podcast_container_state, mock) do
apple_publisher.wait_for_upload_processing([])
end

mock.verify
end
end
end