Draft: Add support to keep a Radarr item that contains a specific tag #204
Conversation
WalkthroughThe pull request introduces a new Radarr configuration option and extends several Radarr-related functionalities. A commented configuration line is added to specify a "skip tag" for bypassing movie deletion in Radarr. Related Scala files are updated to include a new field in the configuration, adjust conversion logic to accommodate a tags list in movies, and modify deletion logic to check for the presence of the skip tag. Additional utility methods in Radarr integrations and test configurations are also modified accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant R as Requester
participant Sync as PlexTokenDeleteSync
participant Utils as RadarrUtils
participant Radarr as Radarr API
R->>Sync: deleteMovie(movie)
Sync->>Sync: Extract TMDB ID from movie
Sync->>Utils: hasSkipMovieTag(apiKey, baseUrl, skipTag, tmdbId)
alt Skip tag exists
Utils-->>Sync: true
Sync->>Sync: Log "deletion will be skipped"
Sync-->>R: Return without deletion
else No skip tag
Utils-->>Sync: false
Sync->>Radarr: deleteFromRadarr(movie)
Radarr-->>Sync: Deletion result
Sync-->>R: Return deletion result
end
Poem
Tip 🌐 Web search-backed reviews and chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Please note, I've never coded in Scala before, so feel free to suggest changes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
src/main/scala/radarr/RadarrConversions.scala (1)
14-14: Consider using Nil instead of List.empty for better performance.While the change correctly adapts to the new RadarrMovie constructor, using
Nilinstead ofList.emptycould provide a minor performance benefit as it avoids object allocation.- RadarrMovie(movie.movieTitle, movie.imdbId, movie.tmdbId, movie.id, List.empty) + RadarrMovie(movie.movieTitle, movie.imdbId, movie.tmdbId, movie.id, Nil)src/main/scala/configuration/Configuration.scala (1)
32-33: Consider adding ScalaDoc to document the new field.While the addition of
radarrSkipTagis well-placed and correctly typed, adding ScalaDoc would help users understand its purpose and expected value.radarrBypassIgnored: Boolean, radarrTagIds: Set[Int], + /** Tag name that, when applied to a Radarr item, prevents its deletion when removed from Plex Watchlist */ radarrSkipTag: Stringsrc/main/resources/config-template.yaml (1)
64-65: Enhance documentation for the skipTag configuration.While the configuration option is well-documented, consider adding more details about the behavior when the tag is present, such as:
- Whether the tag is case-sensitive
- What happens when the tag is present on a movie
- Examples of when to use this feature
Apply this diff to enhance the documentation:
## Tag to use in Radarr to bypass movie deletion +## When this tag is present on a movie in Radarr, the movie will not be deleted +## even if it's removed from the Plex Watchlist +## Example: Set skipTag: "keep" and add the "keep" tag to movies you want to preserve # skipTag: "keep"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/resources/config-template.yaml(1 hunks)src/main/scala/PlexTokenDeleteSync.scala(1 hunks)src/main/scala/configuration/Configuration.scala(1 hunks)src/main/scala/configuration/ConfigurationRedactor.scala(1 hunks)src/main/scala/configuration/ConfigurationUtils.scala(4 hunks)src/main/scala/configuration/Keys.scala(1 hunks)src/main/scala/radarr/RadarrConversions.scala(1 hunks)src/main/scala/radarr/RadarrMovie.scala(1 hunks)src/main/scala/radarr/RadarrTag.scala(1 hunks)src/main/scala/radarr/RadarrUtils.scala(3 hunks)src/test/scala/PlexTokenSyncSpec.scala(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/scala/radarr/RadarrTag.scala
🔇 Additional comments (5)
src/main/scala/radarr/RadarrMovie.scala (1)
3-3: LGTM! The tags field addition is well-structured.The addition of
tags: List[Int]to theRadarrMoviecase class is appropriate for storing tag IDs and aligns with the PR objectives.src/main/scala/configuration/Keys.scala (1)
20-20: LGTM! The configuration key follows established patterns.The addition of
radarrSkipTagconstant maintains consistent naming and formatting with other Radarr configuration keys.src/main/scala/configuration/ConfigurationRedactor.scala (1)
25-25: LGTM!The addition of
radarrSkipTagto the configuration output is consistent with the existing format and supports the new feature.src/main/scala/configuration/ConfigurationUtils.scala (2)
37-37: LGTM!The integration of the skipTag parameter in the radarrConfig tuple and RadarrConfiguration instantiation is well-implemented.
Also applies to: 69-70
192-192: LGTM!The return tuple correctly includes the skipTag parameter.
| _ <- 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix return type mismatch in skip tag handling.
The EitherT.pure[IO, Throwable](None) returns Option[Unit] which doesn't match the expected return type.
- 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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _ <- 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) | |
| } | |
| } | |
| _ <- eitherShouldSkip match { | |
| case Left(error) => | |
| - EitherT.liftF[IO, Throwable, Unit](IO(logger.error(s"Error in hasSkipMovieTag: ${error.getMessage}", error))) | |
| + EitherT.leftT[IO, Unit](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) | |
| + EitherT.pure[IO, Throwable](()) | |
| } else { | |
| 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 | ||
| } |
There was a problem hiding this comment.
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.
- val tmdbId = movie.getTmdbId.getOrElse {
- logger.warn(s"Unable to extract Tmdb ID from movie to delete: $movie")
- 0L
- }
+ movie.getTmdbId.toRight(new IllegalArgumentException(s"Unable to extract TMDB ID from movie: $movie")).toEitherT[IO]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val tmdbId = movie.getTmdbId.getOrElse { | |
| logger.warn(s"Unable to extract Tmdb ID from movie to delete: $movie") | |
| 0L | |
| } | |
| movie.getTmdbId.toRight(new IllegalArgumentException(s"Unable to extract TMDB ID from movie: $movie")).toEitherT[IO] |
| @@ -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.
🛠️ 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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private var skipTagId: Int = -1 | |
| private val skipTagId = new java.util.concurrent.atomic.AtomicInteger(-1) |
| 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.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } yield movies.head.tags.contains(skipTagId) | |
| } yield movies.headOption.map(_.tags.contains(skipTagId)) | |
| .getOrElse(false) |
| 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.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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] |
| if (params != null) { | ||
| params.foreachEntry((k, v) => uri = uri.withQueryParam(k, v)) | ||
| } |
There was a problem hiding this comment.
🛠️ 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))Committable suggestion skipped: line range outside the PR's diff.
| 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) |
There was a problem hiding this comment.
🛠️ 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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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) | |
| private def getToArr[T: Decoder]( | |
| client: HttpClient | |
| )(baseUrl: Uri, apiKey: String, endpoint: String): EitherT[IO, Throwable, T] = | |
| getToArrWithParams(client)(baseUrl, apiKey, endpoint, Map.empty) |
| radarrTagIds = Set(2), | ||
| "keep" |
There was a problem hiding this comment.
Fix RadarrConfiguration constructor parameter order.
The radarrSkipTag parameter is incorrectly placed after radarrTagIds.
- radarrTagIds = Set(2),
- "keep"
+ radarrTagIds = Set(2),
+ radarrSkipTag = "keep"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| radarrTagIds = Set(2), | |
| "keep" | |
| radarrTagIds = Set(2), | |
| radarrSkipTag = "keep" |
| ): 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 |
There was a problem hiding this comment.
Replace orNull with Option for safer null handling.
Using orNull is not idiomatic Scala and could lead to runtime null pointer exceptions. Consider using Option instead.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ): 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 | |
| ): 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) |
Description
Add support to configure a tag that, if such tag is added to the Radarr item, Watchlistarr will skip and not delete that entry from Radarr when the associated movie is removed from Plex Watchlist.
Checklist
sbt scalafmtAllRun (and optionallysbt scalafmtSbt)Summary by CodeRabbit