-
-
Notifications
You must be signed in to change notification settings - Fork 30
Draft: Add support to keep a Radarr item that contains a specific tag #204
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -69,7 +69,30 @@ object PlexTokenDeleteSync extends PlexUtils with SonarrUtils with RadarrUtils { | |||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| private def deleteMovie(client: HttpClient, config: Configuration)(movie: Item): EitherT[IO, Throwable, Unit] = | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (config.deleteConfiguration.movieDeleting) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| deleteFromRadarr(client, config.radarrConfiguration, config.deleteConfiguration.deleteFiles)(movie) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| val tmdbId = movie.getTmdbId.getOrElse { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.warn(s"Unable to extract Tmdb ID from movie to delete: $movie") | ||||||||||||||||||||||||||||||||||||||||||||||||||
| 0L | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| for { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| eitherShouldSkip <- hasSkipMovieTag(client)( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| config.radarrConfiguration.radarrApiKey, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| config.radarrConfiguration.radarrBaseUrl, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| config.radarrConfiguration.radarrSkipTag, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| tmdbId | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ).attempt | ||||||||||||||||||||||||||||||||||||||||||||||||||
| _ <- eitherShouldSkip match { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| case Left(error) => | ||||||||||||||||||||||||||||||||||||||||||||||||||
| EitherT.liftF[IO, Throwable, Unit](IO(logger.error(s"Error in hasSkipMovieTag: ${error.getMessage}", error))) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| case Right(shouldSkip) => | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (shouldSkip) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.info(s"Skipping deletion of movie [${movie}] due to Skip Tag [${config.radarrConfiguration.radarrSkipTag}]") | ||||||||||||||||||||||||||||||||||||||||||||||||||
| EitherT.pure[IO, Throwable](None) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| deleteFromRadarr(client, config.radarrConfiguration, config.deleteConfiguration.deleteFiles)(movie) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+84
to
+94
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Fix return type mismatch in skip tag handling. The - EitherT.pure[IO, Throwable](None)
+ EitherT.pure[IO, Throwable](())Also, consider improving error handling by propagating the error instead of just logging it. - EitherT.liftF[IO, Throwable, Unit](IO(logger.error(s"Error in hasSkipMovieTag: ${error.getMessage}", error)))
+ EitherT.leftT[IO, Unit](error)📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| } yield eitherShouldSkip | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.info(s"Found movie \"${movie.title}\" which is not watchlisted on Plex") | ||||||||||||||||||||||||||||||||||||||||||||||||||
| EitherT.pure[IO, Throwable](()) | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -34,7 +34,7 @@ object ConfigurationUtils { | |||||||||||||
| sonarrBypassIgnored = configReader.getConfigOption(Keys.sonarrBypassIgnored).exists(_.toBoolean) | ||||||||||||||
| sonarrSeasonMonitoring = configReader.getConfigOption(Keys.sonarrSeasonMonitoring).getOrElse("all") | ||||||||||||||
| radarrConfig <- getRadarrConfig(configReader, client) | ||||||||||||||
| (radarrBaseUrl, radarrApiKey, radarrQualityProfileId, radarrRootFolder, radarrTagIds) = radarrConfig | ||||||||||||||
| (radarrBaseUrl, radarrApiKey, radarrQualityProfileId, radarrRootFolder, radarrTagIds, radarrSkipTag) = radarrConfig | ||||||||||||||
| radarrBypassIgnored = configReader.getConfigOption(Keys.radarrBypassIgnored).exists(_.toBoolean) | ||||||||||||||
| plexTokens = getPlexTokens(configReader) | ||||||||||||||
| skipFriendSync = configReader.getConfigOption(Keys.skipFriendSync).flatMap(_.toBooleanOption).getOrElse(false) | ||||||||||||||
|
|
@@ -66,7 +66,8 @@ object ConfigurationUtils { | |||||||||||||
| radarrQualityProfileId, | ||||||||||||||
| radarrRootFolder, | ||||||||||||||
| radarrBypassIgnored, | ||||||||||||||
| radarrTagIds | ||||||||||||||
| radarrTagIds, | ||||||||||||||
| radarrSkipTag | ||||||||||||||
| ), | ||||||||||||||
| PlexConfiguration( | ||||||||||||||
| plexWatchlistUrls, | ||||||||||||||
|
|
@@ -159,8 +160,9 @@ object ConfigurationUtils { | |||||||||||||
| private def getRadarrConfig( | ||||||||||||||
| configReader: ConfigurationReader, | ||||||||||||||
| client: HttpClient | ||||||||||||||
| ): IO[(Uri, String, Int, String, Set[Int])] = { | ||||||||||||||
| ): IO[(Uri, String, Int, String, Set[Int], String)] = { | ||||||||||||||
| val apiKey = configReader.getConfigOption(Keys.radarrApiKey).getOrElse(throwError("Unable to find radarr API key")) | ||||||||||||||
| val skipTag = configReader.getConfigOption(Keys.radarrSkipTag).orNull | ||||||||||||||
|
Comment on lines
+163
to
+165
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replace Using Apply this diff to improve null safety: - ): IO[(Uri, String, Int, String, Set[Int], String)] = {
+ ): IO[(Uri, String, Int, String, Set[Int], Option[String])] = {
val apiKey = configReader.getConfigOption(Keys.radarrApiKey).getOrElse(throwError("Unable to find radarr API key"))
- val skipTag = configReader.getConfigOption(Keys.radarrSkipTag).orNull
+ val skipTag = configReader.getConfigOption(Keys.radarrSkipTag)📝 Committable suggestion
Suggested change
|
||||||||||||||
| val configuredUrl = configReader.getConfigOption(Keys.radarrBaseUrl) | ||||||||||||||
| val possibleUrls: Seq[String] = | ||||||||||||||
| configuredUrl.map("http://" + _).toSeq ++ configuredUrl.toSeq ++ possibleLocalHosts.map(_ + ":7878") | ||||||||||||||
|
|
@@ -187,7 +189,7 @@ object ConfigurationUtils { | |||||||||||||
| .getConfigOption(Keys.radarrTags) | ||||||||||||||
| .map(getTagIdsFromConfig(client, url, apiKey)) | ||||||||||||||
| .getOrElse(IO.pure(Set.empty[Int])) | ||||||||||||||
| } yield (url, apiKey, qualityProfileId, rootFolder, tagIds) | ||||||||||||||
| } yield (url, apiKey, qualityProfileId, rootFolder, tagIds, skipTag) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| private def getTagIdsFromConfig(client: HttpClient, url: Uri, apiKey: String)(tags: String): IO[Set[Int]] = { | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| package radarr | ||
|
|
||
| private[radarr] case class RadarrMovie(title: String, imdbId: Option[String], tmdbId: Option[Long], id: Long) | ||
| private[radarr] case class RadarrMovie(title: String, imdbId: Option[String], tmdbId: Option[Long], id: Long, tags: List[Int]) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| package radarr | ||
|
|
||
| private[radarr] case class RadarrTag(label: String, id: Int) |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,12 +2,11 @@ package radarr | |||||||||||||||||
|
|
||||||||||||||||||
| import cats.data.EitherT | ||||||||||||||||||
| import cats.effect.IO | ||||||||||||||||||
| import cats.implicits._ | ||||||||||||||||||
| import configuration.RadarrConfiguration | ||||||||||||||||||
| import http.HttpClient | ||||||||||||||||||
| import io.circe.{Decoder, Json} | ||||||||||||||||||
| import io.circe.generic.auto._ | ||||||||||||||||||
| import io.circe.syntax.EncoderOps | ||||||||||||||||||
| import io.circe.{Decoder, Json} | ||||||||||||||||||
| import model.Item | ||||||||||||||||||
| import org.http4s.{MalformedMessageBodyFailure, Method, Uri} | ||||||||||||||||||
| import org.slf4j.LoggerFactory | ||||||||||||||||||
|
|
@@ -16,6 +15,8 @@ trait RadarrUtils extends RadarrConversions { | |||||||||||||||||
|
|
||||||||||||||||||
| private val logger = LoggerFactory.getLogger(getClass) | ||||||||||||||||||
|
|
||||||||||||||||||
| private var skipTagId: Int = -1 | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider thread-safety for skipTagId cache. The mutable skipTagId variable could lead to race conditions in a concurrent environment. Consider using an atomic reference or synchronization: - private var skipTagId: Int = -1
+ private val skipTagId = new java.util.concurrent.atomic.AtomicInteger(-1)📝 Committable suggestion
Suggested change
|
||||||||||||||||||
|
|
||||||||||||||||||
| protected def fetchMovies( | ||||||||||||||||||
| client: HttpClient | ||||||||||||||||||
| )(apiKey: String, baseUrl: Uri, bypass: Boolean): EitherT[IO, Throwable, Set[Item]] = | ||||||||||||||||||
|
|
@@ -65,14 +66,47 @@ trait RadarrUtils extends RadarrConversions { | |||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| private def getSkipTagId(client: HttpClient)( | ||||||||||||||||||
| apiKey: String, | ||||||||||||||||||
| baseUrl: Uri, | ||||||||||||||||||
| skipTag: String | ||||||||||||||||||
| ): EitherT[IO, Throwable, Int] = { | ||||||||||||||||||
| if (this.skipTagId >= 0) return EitherT.pure[IO, Throwable](this.skipTagId) | ||||||||||||||||||
| for { | ||||||||||||||||||
| allRadarrTags <- getToArr[List[RadarrTag]](client)(baseUrl, apiKey, "tag") | ||||||||||||||||||
| skipTagId = allRadarrTags.find(t => t.label == skipTag).get.id | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replace unsafe .get with proper error handling. Using .get on Option can throw NoSuchElementException if the tag is not found. - skipTagId = allRadarrTags.find(t => t.label == skipTag).get.id
+ skipTagId <- allRadarrTags.find(t => t.label == skipTag)
+ .toRight(new IllegalArgumentException(s"Skip tag '$skipTag' not found in Radarr"))
+ .map(_.id)
+ .toEitherT[IO]📝 Committable suggestion
Suggested change
|
||||||||||||||||||
| } yield skipTagId | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| protected def hasSkipMovieTag(client: HttpClient)( | ||||||||||||||||||
| apiKey: String, | ||||||||||||||||||
| baseUrl: Uri, | ||||||||||||||||||
| skipTag: String, | ||||||||||||||||||
| tmdbId: Long | ||||||||||||||||||
| ): EitherT[IO, Throwable, Boolean] = { | ||||||||||||||||||
| for { | ||||||||||||||||||
| skipTagId <- this.getSkipTagId(client)(apiKey, baseUrl, skipTag) | ||||||||||||||||||
| movies <- getToArrWithParams[List[RadarrMovie]](client)(baseUrl, apiKey, "movie", Map("tmdbId" -> tmdbId.toString)) | ||||||||||||||||||
| } yield movies.head.tags.contains(skipTagId) | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replace unsafe head with proper error handling. Using head on List can throw NoSuchElementException if the list is empty. - } yield movies.head.tags.contains(skipTagId)
+ } yield movies.headOption.map(_.tags.contains(skipTagId))
+ .getOrElse(false)📝 Committable suggestion
Suggested change
|
||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| private def getToArr[T: Decoder]( | ||||||||||||||||||
| client: HttpClient | ||||||||||||||||||
| )(baseUrl: Uri, apiKey: String, endpoint: String): EitherT[IO, Throwable, T] = | ||||||||||||||||||
| )(baseUrl: Uri, apiKey: String, endpoint: String): EitherT[IO, Throwable, T] = getToArrWithParams(client)(baseUrl, apiKey, endpoint, null) | ||||||||||||||||||
|
Comment on lines
93
to
+95
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Replace null with Option for params. Using null is discouraged in Scala. Consider using Option instead. - )(baseUrl: Uri, apiKey: String, endpoint: String): EitherT[IO, Throwable, T] = getToArrWithParams(client)(baseUrl, apiKey, endpoint, null)
+ )(baseUrl: Uri, apiKey: String, endpoint: String): EitherT[IO, Throwable, T] =
+ getToArrWithParams(client)(baseUrl, apiKey, endpoint, Map.empty)📝 Committable suggestion
Suggested change
|
||||||||||||||||||
|
|
||||||||||||||||||
| private def getToArrWithParams[T: Decoder]( | ||||||||||||||||||
| client: HttpClient | ||||||||||||||||||
| )(baseUrl: Uri, apiKey: String, endpoint: String, params: Map[String, String]): EitherT[IO, Throwable, T] = { | ||||||||||||||||||
| var uri = baseUrl / "api" / "v3" / endpoint | ||||||||||||||||||
| if (params != null) { | ||||||||||||||||||
| params.foreachEntry((k, v) => uri = uri.withQueryParam(k, v)) | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+101
to
+103
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Replace null check with Option handling. Using null checks is discouraged in Scala. - if (params != null) {
- params.foreachEntry((k, v) => uri = uri.withQueryParam(k, v))
- }
+ params.foreachEntry((k, v) => uri = uri.withQueryParam(k, v))
|
||||||||||||||||||
| for { | ||||||||||||||||||
| response <- EitherT(client.httpRequest(Method.GET, baseUrl / "api" / "v3" / endpoint, Some(apiKey))) | ||||||||||||||||||
| response <- EitherT(client.httpRequest(Method.GET, uri, Some(apiKey))) | ||||||||||||||||||
| maybeDecoded <- EitherT.pure[IO, Throwable](response.as[T]) | ||||||||||||||||||
| decoded <- EitherT.fromOption[IO](maybeDecoded.toOption, new Throwable("Unable to decode response from Radarr")) | ||||||||||||||||||
| } yield decoded | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| private def postToArr[T: Decoder]( | ||||||||||||||||||
| client: HttpClient | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -48,7 +48,8 @@ class PlexTokenSyncSpec extends AnyFlatSpec with Matchers with MockFactory { | |||||||||
| radarrQualityProfileId = 1, | ||||||||||
| radarrRootFolder = "/root/", | ||||||||||
| radarrBypassIgnored = false, | ||||||||||
| radarrTagIds = Set(2) | ||||||||||
| radarrTagIds = Set(2), | ||||||||||
| "keep" | ||||||||||
|
Comment on lines
+51
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix RadarrConfiguration constructor parameter order. The radarrSkipTag parameter is incorrectly placed after radarrTagIds. - radarrTagIds = Set(2),
- "keep"
+ radarrTagIds = Set(2),
+ radarrSkipTag = "keep"📝 Committable suggestion
Suggested change
|
||||||||||
| ), | ||||||||||
| PlexConfiguration( | ||||||||||
| plexWatchlistUrls = Set(), | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider enhancing error handling for missing TMDB ID.
Using a default TMDB ID of 0L when extraction fails could lead to unexpected behavior. Consider returning an error instead.
📝 Committable suggestion