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 Delegated Delivery UI updates #1139

Merged
merged 23 commits into from
Nov 7, 2024
Merged

Apple Delegated Delivery UI updates #1139

merged 23 commits into from
Nov 7, 2024

Conversation

kookster
Copy link
Member

@kookster kookster commented Oct 23, 2024

  • Add block rss, and enabled check boxes
  • Get the Apple show ID, select from the API if available

also supports locking feeds down, so this info can't be changed via the UI:

@kookster kookster requested a review from svevang October 23, 2024 20:30
Copy link
Member

@svevang svevang left a comment

Choose a reason for hiding this comment

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

A couple minor notes and question.

app/models/apple/show.rb Show resolved Hide resolved
app/models/feeds/apple_subscription.rb Show resolved Hide resolved
<% end %>
<%= form.label :apple_show_id, "Apple Show ID" %>
</div>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

It seems that we almost have a two step process here. Upload the key, and then select and connect the show. Another way to say it is that the show drowdown will be empty until the key is populated.

Should we hide the apple config stuff (apple show id, sync blocs rss etc) until the key is uploaded?

If we did that, then the show's paged query could serve as a quick validation of the key as well.
Screenshot 2024-10-29 at 11 39 43 AM

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 thought about that, and I was torn on whether or not it was worth it - in the case where a user already knows the apple id, we don't need to make it two steps, and since almost all shows will have an apple id whether they have a subscription channel or not, I didn't want to prevent using a known value until it could be looked up.

I'll try making this conditional/two-step, and see how it goes!

Copy link
Member Author

Choose a reason for hiding this comment

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

Another problem with this design is you can't delete or replace the API keys.

app/controllers/feeds_controller.rb Show resolved Hide resolved
@@ -6,38 +6,66 @@
<div class="card-body" data-controller="apple-key">
<div class="mb-4">
<p><%= t(".description") %></p>
<a href="https://help.prx.org/hc/en-us/articles/26602803552155-Setting-up-Apple-Podcast-Subscriptions-with-Apple-Delegated-Delivery"><%= t(".guide_link") %></a>
<%= t(".guide_link").html_safe %>
Copy link
Member Author

Choose a reason for hiding this comment

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

this is a cosmetic change- help urls should not be embedded in the code, moved it to the translations, and allowed a link with html_safe

</div>
<%= form.fields_for :apple_config do |config_form| %>
<%= config_form.fields_for :key do |key_fields| %>
<% unless config_form.object.persisted? %>
<%= key_fields.file_field :key_file, class: "mb-4 form-control", accept: ".p8, .pem", data: {action: "apple-key#convertFileToKey"} %>
<%= key_fields.file_field :key_file, class: "mb-4 form-control", accept: ".p8, .pem", data: {action: "apple-key#convertFileToKey"}, required: true %>
Copy link
Member Author

Choose a reason for hiding this comment

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

now this is required so you can't create the apple feed without adding a file

<%= key_fields.hidden_field :provider_id, data: {apple_key_target: "provider"} %>
<%= key_fields.hidden_field :key_id, data: {apple_key_target: "key"} %>
<% end %>
<% if config_form.object.persisted? %>
Copy link
Member Author

Choose a reason for hiding this comment

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

within this form, also hides all the other fields until a valid key is uploaded and saved

@@ -779,10 +782,13 @@ en:
ad_zones: Control the types of ads that should be stitched into episodes in this feed.
form_apple_config:
title: Apple Subscriptions
description: In order to publish episodes through Apple's Podcast Connect API, ensure you've delegated access to the PRX account in Podcast Connect.
Copy link
Member Author

Choose a reason for hiding this comment

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

this isn't true - you need an API key from whatever apple account, doesn't have to be the PRX account

Feeds::AppleSubscription.all.each do |feed|
apple_id = feed.apple_sync_log&.external_id
feed.update_attribute!(:apple_show_id, apple_id)
end
Copy link
Member Author

Choose a reason for hiding this comment

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

now this migration adds the apple show id, and fills it from the sync log entries

Copy link
Member

@svevang svevang left a comment

Choose a reason for hiding this comment

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

great! 🤩

app/views/feeds/_form.html.erb Show resolved Hide resolved
app/models/apple/key.rb Show resolved Hide resolved
@kookster kookster merged commit 2f2204d into main Nov 7, 2024
3 checks passed
@kookster kookster deleted the fix/apple_dd branch November 7, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants