Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Media servers + newsletter refactoring #4463

Merged
merged 25 commits into from
Jan 31, 2022

Conversation

sephrat
Copy link
Contributor

@sephrat sephrat commented Jan 10, 2022

  • Merge common code between Plex, Emby and Jellyfin
    • Repository
    • Content
    • Episode
  • Make use of this in NewsletterJob to remove duplicate code
  • Save absolute URL for Plex content in DB (be consistent with Emby and Jellyfin)

@tidusjar
Copy link
Member

tidusjar commented Jan 10, 2022

Oh man this has made my day! This is something I've wanted to do for a long time but never had the time to do it!

Update: Code looking good so far :)

@sephrat
Copy link
Contributor Author

sephrat commented Jan 11, 2022

I just hit a major milestone. I can now focus on actually refactoring the newsletter.
There's probably more to do with abstracting the media servers: there are still some unnecessary differences. However the most needed parts are now reworked and should allow for a clean refactoring.
I'll get back to it later, this was exhausting 😄

@@ -79,66 +79,6 @@ public static void CheckForUnairedEpisodes(SearchTvShowViewModel search)
log.LogError(e, "Exception thrown when attempting to check if something is available");
}

if (epExists != null)
Copy link
Member

Choose a reason for hiding this comment

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

why was all of this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

git diff is a bit stupid, the last bits of this method are still there after the part I removed (l.142/82).
If you were asking about the whole section being removed: the Plex/Jellyfin/Emby methods were merged into a single one which takes IMediaServerEpisode as an input.

@sephrat sephrat changed the title Newsletter job refactoring Media servers + newsletter refactoring Jan 16, 2022
@sephrat sephrat marked this pull request as ready for review January 17, 2022 10:22
@sephrat
Copy link
Contributor Author

sephrat commented Jan 17, 2022

I think it's in a good shape now. I've reassessed the scope of this PR, see my edited OP.
There's still some work to do in removing duplicate code between Emby/Jellyfin/Plex in some other places, but the foundations are there to allow this in a future PR.
Once this gets merged, I'll work on the actual PR I initially intended to work on for the newsletter.

@tidusjar
Copy link
Member

Looks like we have some unit test failures :( I'm planning to do some testing around this soon

@sephrat
Copy link
Contributor Author

sephrat commented Jan 24, 2022

Oops, didn't see that one. Pretty sure it works in productive mode though, I've simplified the URL part when merging Plex with Emby and Jellyfin: they're calculated once upon sync. The tests probably only need to be rewritten to check the URL on sync, not on availability check. I'll look into it.

@tidusjar
Copy link
Member

Oops, didn't see that one. Pretty sure it works in productive mode though, I've simplified the URL part when merging Plex with Emby and Jellyfin: they're calculated once upon sync. The tests probably only need to be rewritten to check the URL on sync, not on availability check. I'll look into it.

That makes sense for new syncs, but what about existing synced data?

@sephrat
Copy link
Contributor Author

sephrat commented Jan 24, 2022

Fair point. It can easily be fixed by forcing a full resync, but I guess it'd be better not to break this. I'll see what I can do. For the record, this is only relevant for Plex - Emby and Jellyfin syncs already store the full URL in the database.

@tidusjar
Copy link
Member

If there's nothing simple, then we can just recommend a clear and full resync. It would be a very temporary issue.

URLs are now fetched from database directly
Solves URL for media synced before absolute URL was saved in PlexServerContent
@sephrat
Copy link
Contributor Author

sephrat commented Jan 24, 2022

I've removed the tests since I could not find a simple way to migrate them into sync tests (sorry), and it's not AvaibilityRule's responsibility anymore.
I managed to find a simple fix for legacy Plex content URL.
Should be all good now.

@tidusjar
Copy link
Member

I've been testing this and awesome work! Almost ready to merge it 🤞

@tidusjar tidusjar merged commit 0ff0a70 into Ombi-app:develop Jan 31, 2022
@sephrat sephrat deleted the newsletter-refactor branch February 1, 2022 17:12
@sephrat sephrat mentioned this pull request Mar 22, 2022
4 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants