Increase item limit during plex token retrieval #85
Conversation
WalkthroughThe recent modifications focus on enhancing the synchronization feature of a Scala application, specifically targeting Plex watchlist data. By integrating RSS feed data and adding pagination to the API requests, these changes aim to streamline and enrich the process of syncing watchlist content, ensuring a more efficient and tailored data retrieval from Plex servers. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (4)
- src/main/scala/PlexTokenDeleteSync.scala (2 hunks)
- src/main/scala/plex/PlexUtils.scala (1 hunks)
- src/test/scala/PlexTokenSyncSpec.scala (1 hunks)
- src/test/scala/plex/PlexUtilsSpec.scala (1 hunks)
Additional comments: 6
src/main/scala/PlexTokenDeleteSync.scala (3)
- 1-1: The import of
fetchWatchlistFromRssis correctly added to enable fetching watchlist data from RSS feeds.- 26-26: The aggregation of items from different sources (
selfWatchlist,othersWatchlist,watchlistData) and passing them tomissingIdsOnPlexis logically sound. It ensures that all relevant items are considered when determining which items are missing on Plex.- 1-4: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [56-56]
The deletion logic in
deleteMovieanddeleteSeriesis dependent on configuration flags (movieDeleting,endedShowDeleting,continuingShowDeleting). This is a good practice as it allows for flexible configuration of the deletion behavior. However, ensure that these configurations are well-documented and validated at startup to prevent unintended deletions.src/test/scala/plex/PlexUtilsSpec.scala (1)
- 69-69: The addition of pagination parameters (
X-Plex-Container-Start=0&X-Plex-Container-Size=300) in the URI for the HTTP GET request is correctly implemented. This ensures that the test aligns with the new functionality introduced inPlexUtils.scala.src/main/scala/plex/PlexUtils.scala (1)
- 56-57: The addition of pagination parameters (
X-Plex-Container-StartandX-Plex-Container-Size) to the Plex API request is correctly implemented. This change allows for fetching a specific subset of items from the watchlist, addressing the PR's objective to increase the item limit during Plex token retrieval.src/test/scala/PlexTokenSyncSpec.scala (1)
- 69-69: The update to include pagination parameters (
X-Plex-Container-Start=0&X-Plex-Container-Size=300) in the URI for the HTTP GET request within the test is correctly implemented. This ensures that the test reflects the changes made to the functionality inPlexUtils.scala.
| @@ -17,10 +18,12 @@ object PlexTokenDeleteSync extends PlexUtils with SonarrUtils with RadarrUtils { | |||
| val result = for { | |||
| selfWatchlist <- getSelfWatchlist(config.plexConfiguration, client) | |||
| othersWatchlist <- if (config.plexConfiguration.skipFriendSync) EitherT.pure[IO, Throwable](Set.empty[Item]) else getOthersWatchlist(config.plexConfiguration, client) | |||
| watchlistDatas <- EitherT[IO, Throwable, List[Set[Item]]](config.plexConfiguration.plexWatchlistUrls.map(fetchWatchlistFromRss(client)).toList.sequence.map(Right(_))) | |||
There was a problem hiding this comment.
The use of sequence on a list of IO actions to fetch watchlist data from multiple RSS feeds is a good practice for handling multiple asynchronous operations concurrently. However, consider handling potential failures for each feed individually to avoid failing the entire operation if one feed is inaccessible.
config.plexConfiguration.plexWatchlistUrls.map(url => fetchWatchlistFromRss(client)(url).attempt).toList.sequence.map(_.collect { case Right(items) => items })| import WatchlistSync.fetchWatchlistFromRss | ||
| import cats.data.EitherT | ||
| import cats.effect.IO | ||
| import cats.implicits._ |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [56-56]
The method missingIdsOnPlex correctly iterates over each item to check if it exists in the watchlist. However, the pattern matching inside the loop could be simplified by extracting common logic, such as the creation of EitherT[IO, Throwable, Unit] for successful checks, outside the match statement.
Consider refactoring to reduce redundancy and improve readability.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [56-56]
The error handling in missingIdsOnPlex uses a generic Throwable for unknown categories, which might obscure the root cause of errors. It's recommended to use more specific exceptions or to include more context in the error message.
EitherT[IO, Throwable, Unit](IO.pure(Left(new IllegalArgumentException(s"Unknown category $c for item ${item.title}"))))| @@ -66,7 +66,7 @@ class PlexUtilsSpec extends AnyFlatSpec with Matchers with PlexUtils with MockFa | |||
| val config = createConfiguration(Set("test-token")) | |||
| (mockClient.httpRequest _).expects( | |||
| Method.GET, | |||
| Uri.unsafeFromString("https://metadata.provider.plex.tv/library/sections/watchlist/all?X-Plex-Token=test-token"), | |||
| Uri.unsafeFromString("https://metadata.provider.plex.tv/library/sections/watchlist/all?X-Plex-Token=test-token&X-Plex-Container-Start=0&X-Plex-Container-Size=300"), | |||
There was a problem hiding this comment.
While the test correctly includes the new pagination parameters, it's important to also verify the behavior when different values for these parameters are used. Consider adding tests to cover scenarios with different starting points and sizes to ensure the pagination logic works as expected across a range of values.
Suggest adding parameterized tests to cover a variety of pagination scenarios.
| .withQueryParam("X-Plex-Container-Start", 0) // todo: pagination | ||
| .withQueryParam("X-Plex-Container-Size", 300) |
There was a problem hiding this comment.
While the hardcoded values for pagination (X-Plex-Container-Start=0 and X-Plex-Container-Size=300) are a good starting point, consider making these values configurable or dynamically adjustable based on the total number of items in the watchlist. This would allow for more flexibility and efficiency in data retrieval.
Suggest adding configuration options or logic to adjust pagination parameters dynamically.
| @@ -66,7 +66,7 @@ class PlexTokenSyncSpec extends AnyFlatSpec with Matchers with MockFactory { | |||
| private def defaultPlexMock(httpClient: HttpClient): HttpClient = { | |||
| (httpClient.httpRequest _).expects( | |||
| Method.GET, | |||
| Uri.unsafeFromString("https://metadata.provider.plex.tv/library/sections/watchlist/all?X-Plex-Token=plex-token"), | |||
| Uri.unsafeFromString("https://metadata.provider.plex.tv/library/sections/watchlist/all?X-Plex-Token=plex-token&X-Plex-Container-Start=0&X-Plex-Container-Size=300"), | |||
There was a problem hiding this comment.
As with the PlexUtilsSpec.scala file, consider adding tests to cover different pagination scenarios to ensure the functionality works correctly across a range of values for X-Plex-Container-Start and X-Plex-Container-Size.
Suggest adding parameterized tests to cover various pagination scenarios.
Partial fix for #79
Summary by CodeRabbit