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.

Sign up for GitHub

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

Jump to bottom

Increase item limit during plex token retrieval #85

Merged
merged 1 commit into from
Jan 29, 2024
Merged

Increase item limit during plex token retrieval #85

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/main/scala/PlexTokenDeleteSync.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import WatchlistSync.fetchWatchlistFromRss
import cats.data.EitherT
import cats.effect.IO
import cats.implicits._
Comment on lines +1 to 4
Copy link

Choose a reason for hiding this comment

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

📝 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}"))))

Expand All @@ -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(_)))
Copy link

Choose a reason for hiding this comment

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

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 })

watchlistData = watchlistDatas.flatten.toSet
moviesWithoutExclusions <- fetchMovies(client)(config.radarrConfiguration.radarrApiKey, config.radarrConfiguration.radarrBaseUrl, bypass = true)
seriesWithoutExclusions <- fetchSeries(client)(config.sonarrConfiguration.sonarrApiKey, config.sonarrConfiguration.sonarrBaseUrl, bypass = true)
allIdsWithoutExclusions = moviesWithoutExclusions ++ seriesWithoutExclusions
_ <- missingIdsOnPlex(client)(config)(allIdsWithoutExclusions, selfWatchlist ++ othersWatchlist)
_ <- missingIdsOnPlex(client)(config)(allIdsWithoutExclusions, selfWatchlist ++ othersWatchlist ++ watchlistData)
} yield ()

result.leftMap {
Expand Down
2 changes: 2 additions & 0 deletions src/main/scala/plex/PlexUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ trait PlexUtils {
val url = Uri
.unsafeFromString("https://metadata.provider.plex.tv/library/sections/watchlist/all")
.withQueryParam("X-Plex-Token", token)
.withQueryParam("X-Plex-Container-Start", 0) // todo: pagination
.withQueryParam("X-Plex-Container-Size", 300)
Comment on lines +56 to +57
Copy link

Choose a reason for hiding this comment

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

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.


for {
response <- EitherT(client.httpRequest(Method.GET, url))
Expand Down
2 changes: 1 addition & 1 deletion src/test/scala/PlexTokenSyncSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Copy link

Choose a reason for hiding this comment

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

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.

None,
None
).returning(IO.pure(parse(Source.fromResource("self-watchlist-from-token.json").getLines().mkString("\n")))).once()
Expand Down
2 changes: 1 addition & 1 deletion src/test/scala/plex/PlexUtilsSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Copy link

Choose a reason for hiding this comment

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

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.

None,
None
).returning(IO.pure(parse(Source.fromResource("self-watchlist-from-token.json").getLines().mkString("\n")))).once()
Expand Down
Loading