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

feat: remove requests of unmonitored movies/seasons during scan #817

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

bonswouar
Copy link

@bonswouar bonswouar commented Jun 14, 2024

Description

Quick & easy first implementation of Monitoring status support in Radarr:
When a movie - which was monitored before - is unmonitored, it won't appear as "requested" in Jellyseerr anymore

Issues Fixed or Closed

@bonswouar bonswouar requested a review from Fallenbagel as a code owner June 14, 2024 21:39
@bonswouar bonswouar changed the title feat: set movie status to unknown if unmonitored from radarr during scan feat: set movie status to unknown if unmonitored from radarr during scan [WIP] Jun 14, 2024
@Fallenbagel Fallenbagel marked this pull request as draft June 15, 2024 04:50
@Fallenbagel
Copy link
Owner

Converted to draft as the title states "WIP". Once it's ready feel free to make this pr ready for review

@bonswouar
Copy link
Author

@Fallenbagel Thanks I didn't think about the Draft feature

This is a WIP but it's basically working as I'd expect.
But I could update it with a more specific Monitoring status support if you think it's a good idea?

Only drawback is that if you have lots of Unmonitored movies in Radarr the scan should be slower, as it will request each unmonitored movie in db every time. Let me know if you have some thoughts about this

@bonswouar
Copy link
Author

FYI I've been using this for months and it works as expected.
I suspect some other users might need this functionality, couldn't we merge it?

A more complete monitoring status support wouldn't be very useful imo, at least I didn't encounter any use case

@bonswouar bonswouar changed the title feat: set movie status to unknown if unmonitored from radarr during scan [WIP] feat: set movie status to unknown if unmonitored from radarr during scan Sep 27, 2024
@gauthier-th gauthier-th marked this pull request as ready for review September 27, 2024 14:10
@gauthier-th gauthier-th self-requested a review as a code owner September 27, 2024 14:10
Copy link
Collaborator

@gauthier-th gauthier-th left a comment

Choose a reason for hiding this comment

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

I don't think this should be merged if it's only Radarr. Could you implement the same logic for Sonarr seasons?
And shouldn't there be a setting to enable/disable this behavior? You may have unmonitored a movie/tv show but still want to keep it as requested.

@gauthier-th gauthier-th linked an issue Oct 6, 2024 that may be closed by this pull request
1 task
Copy link

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.

@github-actions github-actions bot added the merge conflict Cannot merge due to merge conflicts label Nov 10, 2024
@github-actions github-actions bot removed the merge conflict Cannot merge due to merge conflicts label Nov 10, 2024
@bonswouar
Copy link
Author

bonswouar commented Nov 10, 2024

@gauthier-th I've added the Setting, disabled by default

Also I've tried to add the same behaviour for Sonarr, but I encountered an issue:

  • Some (specific) seasons are still shown as "Requested", I don't understand how as I get the new log message "Removing request for ..." for those. Plus I've added a check in processShow() directly, so I really don't see where that could happen in the code. If you've got any clue?

Also, not really related to this issue afaik, but still looks like the same kind of problem:
I've noticed during my debugging that some Seasons are shown as "Available" although they don't contain any file in Sonarr (and are not monitored).
I'd guess it's related to If the season is already marked as available, we force it to stay available (to avoid competing scanners), but I'm really not sure why this condition exists despite the comment (why not at least check if it has any episode instead of relying entirely on the previous status?!)

@bonswouar bonswouar changed the title feat: set movie status to unknown if unmonitored from radarr during scan feat: remove requests of unmonitored movies/seasons during scan Nov 10, 2024
@bonswouar
Copy link
Author

I'll squash the commits when everything's approved

Meanwhile any hint on Sonarr weird behaviour is welcome! I probably missed something, don't know much about the project nor Typescript

Copy link
Collaborator

@gauthier-th gauthier-th left a comment

Choose a reason for hiding this comment

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

It looks like the way things are done are not consistent between Radarr and Sonarr.
Why do you change the status of the Media entity for movies while you remove the SeasonRequest for series?
It may be linked to the issue you experienced.

src/i18n/locale/fr.json Outdated Show resolved Hide resolved
src/components/Settings/SettingsMain/index.tsx Outdated Show resolved Hide resolved
server/lib/scanners/sonarr/index.ts Outdated Show resolved Hide resolved
server/lib/scanners/sonarr/index.ts Outdated Show resolved Hide resolved
Comment on lines 288 to 295
: settings.main.removeUnmonitoredFromRequestsEnabled &&
!season.monitored &&
season.episodes == 0
? MediaStatus.UNKNOWN
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this change necessary? Isn't the Sonarr scanner job enough?

Copy link
Author

@bonswouar bonswouar Nov 11, 2024

Choose a reason for hiding this comment

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

It seems to be necessary. It is part of Sonarr scanner job though!

  • At the beginning of the job I check if there is any SeasonRequest and delete it if necessary
  • And here I check the Status of the Season, as every Season is parsed and its Status checked/updated, it should set un-monitored season with no file to UNKNOWN (as for Movies)

@gauthier-th
Copy link
Collaborator

Also, not really related to this issue afaik, but still looks like the same kind of problem: I've noticed during my debugging that some Seasons are shown as "Available" although they don't contain any file in Sonarr (and are not monitored). I'd guess it's related to If the season is already marked as available, we force it to stay available (to avoid competing scanners), but I'm really not sure why this condition exists despite the comment (why not at least check if it has any episode instead of relying entirely on the previous status?!)

There are several jobs that update the status of a Media: Radarr/Sonarr jobs, and Jellyfin/Emby/Plex jobs, hence the comment about competing jobs.

@bonswouar
Copy link
Author

bonswouar commented Nov 11, 2024

It looks like the way things are done are not consistent between Radarr and Sonarr. Why do you change the status of the Media entity for movies while you remove the SeasonRequest for series? It may be linked to the issue you experienced.

For clarity, there are two potential "problems" with un-monitored medias that I've been trying to fix:

  1. Scenario 1
    Nothing is requested. But the movie was originally added (from Radarr directly) with the status monitored. It's been un-monitored later

    • In Jellyseerr it will always be shown as "Requested", which imo is a real issue (it really doesn't match Radarr behaviour, and the user can't know nor do anything about it, will never be able to really request it, etc)
      • => That's my first commit specifically for this problem, for Movies only (and there is no Request in this case, despite the visible "Requested" status)
  2. Scenario 2
    Something is requested, but manually un-monitored before it completes. For Movies it's not really a problem imo, as the admin can as easily delete a request from Jellyseerr than un-monitor a Movie from Radarr. But for Shows it becomes more complicated as an admin can't EDIT a Request

    • Thus if for example multiple Seasons of a Show are Requested, but only 1 kept & all the other ones unmonitored from Sonarr, the user won't be able to request again the other ones.
      • => That's the main goal of my second commit for Shows

I can certainly add 1/ for Shows for consistency, but not sure many users automatically import monitored Shows from Sonarr and un-monitor them before they're downloaded. Actually I'm pretty sure that's also what this change does: Season not monitored && no episode = Status UNKNOWN
I hope my explanation & use cases do make sense!

@bonswouar
Copy link
Author

bonswouar commented Nov 11, 2024

There are several jobs that update the status of a Media: Radarr/Sonarr jobs, and Jellyfin/Emby/Plex jobs, hence the comment about competing jobs.

I still don't get the logic, why "If the season is already marked as available, we force it to stay available"?
What if the season is not available anymore??!
The referencing code is:

(season.totalEpisodes === season.episodes && season.episodes > 0) ||
            existingSeason.status === MediaStatus.AVAILABLE

And I don't get why we don't check anything on this second line (i.e existingSeason.status === MediaStatus.AVAILABLE && season.episodes > 0)
But anyway that should be a separate issue I guess as it's not about monitor status

EDIT: Well I did try this simple change, and it seems to fix this issue for one season that was always shown as Available before (although it has been deleted some time ago)

@bonswouar
Copy link
Author

bonswouar commented Nov 11, 2024

@gauthier-th I went with your recommendations, and marked as resolved the comments I was sure about.
I'll let you have a look at my other answers

Not sure why/how, but during my last tests the problem seems fixed!
Although, after running 2 Sonarr scans, I still had:

Removing request for Season X of tmdbId XXXX as it is unmonitored

with the same Season/ID, twice in the log. Which means the first time didn't work, right?

Is there something I should know about async deletion in typescript or something? This seems a bit random

@gauthier-th
Copy link
Collaborator

I still don't get the logic, why "If the season is already marked as available, we force it to stay available"?
What if the season is not available anymore??!

There is another job, "Media Availability Sync" that runs and mark the media as unavailable if necessary.

Is there something I should know about async deletion in typescript or something? This seems a bit random

It could come from your forEach loops. For instance in the processUnmonitoredSeason you are using an async function inside a forEach loops, but forEach loops don't wait the end of the async function to run the next item. IMHO It's better to use a for loop to iterate over an array than a forEach, because of this async function concurrency.

Let me review it one more time with the change you made and with your comments in mind.

server/lib/scanners/baseScanner.ts Outdated Show resolved Hide resolved
@bonswouar
Copy link
Author

@gauthier-th I've updated with for loops, rebased from develop, and squashed everything into one cleaner commit.
Please let me know if some other changes are needed!

@bonswouar bonswouar requested a review from gauthier-th December 8, 2024 18:18
@gauthier-th
Copy link
Collaborator

Is the issue with the log message repeated twice fixed?

@bonswouar
Copy link
Author

bonswouar commented Dec 9, 2024

Is the issue with the log message repeated twice fixed?

After more tests... Apparently not :/
It seems await seasonRequestRepository.remove(requestedSeason); still has no effect (sometimes?!), i.e I have only 1 Season of 1 TV Show requested & deleted & unmonitored, and it still shows this message for every Sonarr scan, and this (season / show) request is never deleted at all.

Just for clear context, the log & remove is pretty simple:

this.log(
   `Removing request for Season ${season.seasonNumber} of tmdbId ${tmdbId} as it is unmonitored`
);
await seasonRequestRepository.remove(requestedSeason);

I've tried without the await, same issue.
My guess is that part of the problem might be because processShow is happening after processUnmonitoredSeason, with the "removed season" included ; but that seems weird that the request doesn't get removed still!

I've also tried a dirty workaround (skip adding the season to processableSeasons), but that didn't help either


What's next could be a new Issue, but just in case it's related..

I forgot about that, but as pointed out in one of my previous comment the issue for this specific case is also that the Season is still marked as Available, although it's been deleted for some time now.
And if you're wondering, yes, Media Availability Sync job ran many times since this season got removed!

Could it be that Jellyseerr relies only Jellyfin's "basic season status" to check the status of a Season?
My guess would be:
As the (empty) season folder still exists, in Jellyfin the Season is still shown, but is empty. In Sonarr its status makes sense though, that's why I was expecting it to be based on Sonarr.. Or at least to check if there are any episodes available
Sounds a bit like this issue, but I have no idea what "misconfiguration" it could be (plus the "Play on Jellyfin" do link to the Season page, it just contains no episode).

I wanted to give a try with the official image to confirm I have the same issue, but got the error no such column: User_settings.region, that got my instance offline, apparently related to the latest commit.
I can't do much more tests right now, as the only instance I have is my "production" one

@bonswouar bonswouar force-pushed the radarr-monitoring branch 2 times, most recently from ece1550 to caaf13e Compare December 9, 2024 22:33
@bonswouar
Copy link
Author

I now realize that this last test (and all other similar weird behaviour I had with deleted seasons) is out of scope of this PR.... It's only a Media Availability Sync issue.

Still I don't understand why remove does not remove, but it's not related to "an unmonitored tv show", only a "deleted tv show".

@gauthier-th
Copy link
Collaborator

I may have an idea. Your processUnmonitoredMovie and processUnmonitoredSeason are not consistent.

In the availability sync job, we don't really delete the media, but we set its status as UNKNOWN, for movies and shows. That's what you're doing in processUnmonitoredMovie, but not in processUnmonitoredSeason, you don't update the status of the seasons.

Also, you don't remove the request for the movie, but you do remove it for shows.

Try to update these 2 functions to mimic the behavior of mediaUpdater and seasonUpdater in the availability sync job.

@gauthier-th
Copy link
Collaborator

gauthier-th commented Dec 11, 2024

@Fallenbagel any idea why the media availability sync job mark the movies/seasons as MediaStatus.UNKNOWN instead of deleting the media?
We prefer not to permanently delete requests because retaining them, even when marked as deleted, makes it easier to debug issues like the media availability sync job silently removing requests, so we can trace history and understand what happened.

@bonswouar
Copy link
Author

bonswouar commented Dec 11, 2024

I understand!
Although I believe I've mixed things because of those shows that still appear as "Available" even if they're not anymore (meaning, they've been deleted since!).. But that's a totally different issue, the availability sync job should take care or that, no matter if they're monitored or not.
I'll remove that part from my PR and will let you know if it seems to make sense afterwards, thanks for the feedbacks!

And about my "scenario 2" of this previous comment, I guess it's out of scope of this PR, and could be fixed / worked around by another feature request, for example admins being able to edit requests (something like #585)

Copy link

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.

@github-actions github-actions bot added the merge conflict Cannot merge due to merge conflicts label Dec 29, 2024
@github-actions github-actions bot removed the merge conflict Cannot merge due to merge conflicts label Jan 11, 2025
@bonswouar
Copy link
Author

bonswouar commented Jan 11, 2025

@gauthier-th FYI I've removed the part about TV Shows from my commit (basically went back to my first iteration...)

I don't know how to manage unmonitored shows. Should we check for seasons only, or the whole show? Should we also set it (every episodes, or..?!) to MediaStatus.UNKNOWN? Etc.

As a reminder, the original bug (it was more a bug fix than a new feature imo) was that:
Movies added (and monitored) in Radarr appear as "requested" (although there was no actual request), and stay forever as requested if not downloaded, even if unmonitored (preventing actual users to really request them!).

This is mainly due to Radarr's Import Lists, which is a very useful feature imo (I'd even say that's why I've started to use Radarr in the first place).
But that could also happen if something is manually monitored on Radarr, and then unmonitored.

But I guess this is not something that should happen with Shows, I don't think anybody adds monitored TV Shows automatically with Import List in Sonarr (at least I don't), and I actually have no idea if this bug also exists as I don't always understand the logic behind "requested" in Jellyseerr (and I don't have any test env, I don't want to mess with my production env again).

I understand if you prefer to close this PR, I'll continue to use my fork anyway

@Fallenbagel
Copy link
Owner

Fallenbagel commented Jan 11, 2025

Movies added (and monitored) in Radarr appear as "requested" (although there was no actual request), and stay forever as requested if not downloaded, even if unmonitored (preventing actual users to really request them!).

Movies added and monitored in radarr will not show as requested. Can you test this. We don't do bidirectional sync like this. There Was a bug where it deleted requests making it appear as so but this is not the case and this never was the case. If you believe so could you record and show me after adding a movie as monitored (a new movie) and then show me that exact same movie as marked as requested on jellyseerr?

Asking because I might have missed something so I want to confirm before proceeding. I want to be able to reproduce this if it's a confirmed bug.

@bonswouar
Copy link
Author

bonswouar commented Jan 11, 2025

Movies added and monitored in radarr will not show as requested. Can you test this. We don't do bidirectional sync like this.

Well I've just checked some random recently auto-imported (and monitored) movies in Radarr, I confirm it's still the case.

But that can make sense, specifically when they're unreleased: users shouldn't need/be able to request movies that will already be automatically downloaded when they're available I guess (at least in current Jellyseer's state; it could be useful if Movies can be requested by more than 1 user though).
I don't know if this was a bug that they originally appear as requested, but I like that users can focus to request only really-not-monitored movies!

@bonswouar
Copy link
Author

bonswouar commented Jan 11, 2025

If you believe so could you record and show me after adding a movie as monitored (a new movie) and then show me that exact same movie as marked as requested on jellyseerr?

So the ones added today don't appear as Requested as the sync didn't happen yet.
But added yesterday for example:

image
image

(But again, I think in current state of thing this is a wanted behaviour, as Monitored status isn't shown. Otherwise users will request "for nothing" all the unreleased movies that are already monitored! Be able to request an already-monitored movie could make sense only if more than 1 user is able to request a movie imo - they could then be seen more like a "follow" movie)

@Fallenbagel
Copy link
Owner

If you believe so could you record and show me after adding a movie as monitored (a new movie) and then show me that exact same movie as marked as requested on jellyseerr?

So the ones added today don't appear as Requested as the sync didn't happen yet. But added yesterday for example:

image image

(But again, I think in current state of thing this is a wanted behaviour, as Monitored status isn't shown. Otherwise users will request "for nothing" all the unreleased movies that are already monitored! This could make sense only if more than 1 user is able to request a movie imo - requests could then be seen more like a "follow" movie)

Oh so it does change the status. I see. iirc it wasn't like this previously as I remember being able to request ones already added to radarr (but i guess this was changed recently and since I haven't looked into it yet did not notice it). I will have a look at it.

@bonswouar
Copy link
Author

bonswouar commented Jan 11, 2025

iirc it wasn't like this previously as I remember being able to request ones already added to radarr (but i guess this was changed recently and since I haven't looked into it yet did not notice it)

This has been like that since I use Jellyseer I believe, so more than 1 year. But for sure at least since I've reported the issue (of movies that still appear as requested even after being unmonitored), so since April

If you "fix" that behavior, users will be even less informed about what's monitored or not though, right?

@Fallenbagel
Copy link
Owner

This has been like that since I use Jellyseer I believe, so more than 1 year. But for sure at least since I've reported the issue (of movies that still appear as requested even after being unmonitored), so since April

We started as a fork during 2022. IIRC it did not work like that because I remember support requests about it (but i could be misremembering as well)

If you "fix" that behavior, users will be even less informed about what's monitored or not though, right?

Yes. Which is why i said I will look into it for a better way to handle this without requiring major refactors

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.

Support monitor type for Radarr
3 participants