Handle feed inclusion delegated delivery UI #1384
Conversation
kookster
left a comment
kookster
left a comment
There was a problem hiding this comment.
Looks great, esp all the tests.
There are a couple of places I suggested having another look - where Apple code seems to match generic integration code and maybe can be consolidated, and a place where a generic integration method now has apple specific logic?
| "complete" | ||
| else | ||
| "not_found" | ||
| "complete" |
There was a problem hiding this comment.
Changing to a default of complete? I guess I worry that could show complete even when it is not?
There was a problem hiding this comment.
Checking both possibilities for delivered? boolean means that the else branch was unreachable code:
feeder.prx.org/app/helpers/episodes_helper.rb
Lines 26 to 29 in b1b6d6b
Hence the rearrangement of the apple episode publishing invocation in this PR https://github.com/PRX/feeder.prx.org/pull/1384/files#diff-a2b7e8c0371fceca58ba0b83a4db37ef232d2770af6aeb616aaac0e3b0259876R148. Sort of a subtle difference, but I think makes sense to see delivered episodes as being "complete" here (already published).
| def integration_error_state?(integration) | ||
| case integration | ||
| when :apple | ||
| apple_episode&.audio_asset_state_error? || apple_episode&.delivery_file_errors? |
There was a problem hiding this comment.
The rest of this concern is generic to any integration, could we have this call a generic method that could be implemented by any of the integrations, and then have that implemented for apple with this check, and in megaphone (or in a default episode) just return false?
Probably we should also introduce a more generic error status on the episode delivery status record, like if the latest update returned a 50x we could set that and show we're getting errors back from the integration API calls? That seems like a different issue/PR - right now I think an error back from Megaphone would show up as an incomplete or processing state, assuming it would be retried
There was a problem hiding this comment.
could we have this call a generic method that could be implemented by any of the integrations
Yep. I'm in a functional mindset where I wanted to see the complete projection of state on the branches, but def this is not good OOP style. Will extract this into helpers!
we should also introduce a more generic error status on the episode delivery status record
Yep, also thinking about state, and that discussion we had earlier about stale / drifted state on the sync on the episode publishing status (related to API errors).
| end | ||
|
|
||
| def integration_episode(integration) | ||
| integration_episode_method = "#{integration}_episode" |
| apple_episode&.audio_asset_state_error? || apple_episode&.delivery_file_errors? | ||
| integration_episode_method = "#{integration}_episode" | ||
| if respond_to?(integration_episode_method) | ||
| send(integration_episode_method)&.error_state? || false |
| include TextSanitizer | ||
| include EmbedPlayerHelper | ||
| include AppleDelivery | ||
| include AppleIntegration |
| <%= local_time_ago(episode_integration_updated_at(integration, episode)) %> | ||
| </p> | ||
| </div> | ||
| <% end %> |
closes #1354
This PR improves how episode integration statuses are displayed in the UI, particularly for feeds using delegated delivery where not all episodes are published to external integrations. It also
delivered?then it is marked as published in Apple's system.integration_feed_episode?. This prevents showing misleading "processing" or "incomplete" statuses for episodes that will never be published to an integration.