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

Integrations interface #1136

Closed
wants to merge 1 commit into from
Closed

Integrations interface #1136

wants to merge 1 commit into from

Conversation

svevang
Copy link
Member

@svevang svevang commented Oct 21, 2024

Sets up a new interface that hopes to bring coupling across integration interfaces. This focuses on two major points of contact:

  1. The set operations on the episodes, addresses episodes that are rolling through their CRUD lifecycle
  2. The relationship between feeds and the integration. The approach here is to use the same pattern as the Delegated Delivery integration and focus on the private+default feeds

@svevang svevang marked this pull request as ready for review October 21, 2024 21:23
@svevang svevang requested a review from kookster October 21, 2024 22:05
Copy link
Member Author

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

The overall code layout here is not super important -- i.e. using inheritance versus modules etc. The way this interface was extracted was basically to make a 1:1 with the existing DD code. I tried to make it plausible, but the organization is not super critical.

What is important stuff is over here (bag o' methods):

  1. the Show's handle on the root of the episode sets https://github.com/PRX/feeder.prx.org/pull/1136/files#diff-7e63a5638b6bcfc6976d59f81c02fd900a2f4d47e105962571a44f5cf89fc576
  2. the filtering and sorting of episodes in the Episode class: https://github.com/PRX/feeder.prx.org/pull/1136/files#diff-92e7117a8391a2f8207842a2c3ac800f2d62db2e82150c9270b95426f5d5da66

@@ -0,0 +1,56 @@
module Integrations
module Base
module EpisodeSetOperations
Copy link
Member Author

Choose a reason for hiding this comment

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

The root of the episode set manipulation. The consumption side: Deriving the sets of episodes and filtering them for CRUD sync

@@ -0,0 +1,50 @@
module Integrations
module Base
class Show
Copy link
Member Author

Choose a reason for hiding this comment

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

The show gives the handle on the episodes. The production side: baseline unfiltered, sorted set of episodes.

Copy link
Member

@kookster kookster left a comment

Choose a reason for hiding this comment

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

I'm merging this into the megaphone branch and trying to make use of some of this.
There are some apple-ish ideas in here still, and names of things, which do not match megaphone, or likely other integrations, "show" which is only an Apple concept.

I think the set operations on episodes and comparing all episodes in the podcast to what belongs in the feed make sense, as does a publisher object responsible for making changes to individual episodes to match those sets.

The way the api integration is built to apple is different form how I already built the megahone api entities, so those integrations.base.show and episode may not be as useful.

What I am wishing I had was a common way to maintain state, like the episode status, that says if an episode is sync'd or not and to what version of audio. I think I can refactor the apple episode statuses model for this, and make it more generic.

I also wish there was a common way to record external ids related to an integration.

The sync log is close, but linking the data to the parent feed instead of the private/integration feed is an odd choice, and it lacks awareness of being an apple specific log at the moment (though adding a column could help with that).

I'm not sure why the sync log records all the data based on the public feed, when the private feed has all the other configuration info and data related to apple?
It also means a podcast cannot sync to more than one show on apple.

idk if the refactoring necessary to make these work for another (megaphone) integration will be too time consuming and extensive, I'll have to get into it and see. If not, I'll have to add different tables to maintain state.

What makes more sense to me is to consider the feed as the integration, and so storing external id of the podcast to the integration can be on the feed itself, and then the episode_feeds join table or an entity related to it makes sense to me as a place to store the external_id for the episode in that integration.

The sync log is sorta close to that, but normalized to store a multitude of entities, and currently is associated with the wrong feed id (the public one, not the integration/private feed).

end

# Delegate methods to feeder_episode
def method_missing(method_name, *arguments, &block)
Copy link
Member

Choose a reason for hiding this comment

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

The episode integration written for megaphone act as their own entity, passing through methods to the underlying feeder episode is likely to cause issues, this is not needed.

raise NotImplementedError, "Subclasses must implement archived?"
end

def video_content_type?
Copy link
Member

Choose a reason for hiding this comment

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

Megaphone doesn't support video at all, so this is also not applicable

Copy link
Member

Choose a reason for hiding this comment

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

my bad, that is how this is used! perfect

raise NotImplementedError, "Subclasses must implement synced_with_integration?"
end

def integration_new?
Copy link
Member

Choose a reason for hiding this comment

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

this means not persisted to the integration?

it would be helpful of there were some common way for integrations to save external ids and state of persistence to the 3rd party

# or if the episode is a video
eps
.reject(&:synced_with_integration?)
.reject(&:video_content_type?)
Copy link
Member

Choose a reason for hiding this comment

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

ah, that is common to megaphone as well, but that's sorta luck - other integrations may have no issue with video, so this method may not be common/reusable

def filter_episodes_to_archive(eps, eps_in_feed)
# Episodes to archive can include:
# - episodes that are now excluded from the feed
# - episodes that are deleted or unpublished
Copy link
Member

Choose a reason for hiding this comment

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

Maybe? megaphone doesn't have an "archived" status for episodes, that is an apple concept
It has a notion of publishing or unpublishing- but the status is one of "published", "scheduled", "not_ready"

attr_reader :show

def initialize(show:)
@show = show
Copy link
Member

Choose a reason for hiding this comment

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

A "show" is an apple concept, not in megaphone, in megaphone it is a Podcast.
The api podcast object for megaphone could maybe implement some of this, we'll see, but at a minimum it's a misnomer, and not how I organized the megaphone code

filter_episodes_to_unarchive(show.episodes)
end

def publish!
Copy link
Member

Choose a reason for hiding this comment

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

This makes sense as it is the only part of this that I see called by another class/module, this seems like the interface of the publisher

attr_reader :feed

def initialize(public_feed:, private_feed:)
@public_feed = public_feed
Copy link
Member

Choose a reason for hiding this comment

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

public feed not used in this

@svevang
Copy link
Member Author

svevang commented Dec 2, 2024

closing per #1112

@svevang svevang closed this Dec 2, 2024
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