Jellyfin notifier option #8730
Jellyfin notifier option #8730
Conversation
WalkthroughThis update expands SickChill's functionality by integrating Jellyfin, enabling users to seamlessly connect their media library with Jellyfin for automatic updates and notifications. 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 review |
![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: 5
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
sickchill/gui/slick/images/16px_jellyfin_notifier.pngis excluded by:!**/*.pngsickchill/gui/slick/images/32px_jellyfin_notifier.pngis excluded by:!**/*.png
Files selected for processing (13)
- sickchill/gui/slick/css/style.css (2 hunks)
- sickchill/gui/slick/js/core.js (1 hunks)
- sickchill/gui/slick/views/config_notifications.mako (1 hunks)
- sickchill/gui/slick/views/layouts/main.mako (1 hunks)
- sickchill/oldbeard/notifiers/init.py (2 hunks)
- sickchill/oldbeard/notifiers/jellyfin.py (1 hunks)
- sickchill/oldbeard/postProcessor.py (1 hunks)
- sickchill/oldbeard/show_queue.py (1 hunks)
- sickchill/oldbeard/stevedore.py (1 hunks)
- sickchill/settings.py (2 hunks)
- sickchill/start.py (3 hunks)
- sickchill/views/config/notifications.py (2 hunks)
- sickchill/views/home.py (4 hunks)
Additional comments: 23
sickchill/oldbeard/notifiers/__init__.py (2)
- 12-12: Adding
jellyfinto the list of imported notifiers is a necessary step for integrating Jellyfin as a notifier option. This change correctly follows the pattern established for other notifiers in the system.- 39-39: Creating an instance of
jellyfin.Notifier()aligns with the instantiation pattern for other notifiers in the system. This ensures that Jellyfin notifications can be utilized in the same manner as other notifiers.sickchill/oldbeard/notifiers/jellyfin.py (4)
- 10-13: The
_make_headersmethod correctly constructs headers for Jellyfin API requests, including the necessaryX-Emby-Tokenfor authentication. Usingsettings.JELLYFIN_APIKEYas a default whenjellyfin_apikeyis not explicitly provided is a good practice for flexibility.- 14-36: The
_notify_jellyfinmethod properly handles sending notifications to Jellyfin. It checks if Jellyfin notifications are enabled (settings.USE_JELLYFIN) before proceeding, which is a good practice. However, consider adding a log message or raising an exception when the response status code is not 204 (No Content), indicating that the request was successful but Jellyfin did not return any content. This would help in debugging issues where Jellyfin does not process the notification as expected.if response.status_code != 204: - logger.debug(_("JELLYFIN: Notification sent successfully.")) + logger.warning(_("JELLYFIN: Expected status code 204, got {status_code}. Check Jellyfin server.").format(status_code=response.status_code))
- 42-44: The
test_notifymethod provides a way to send a test notification, which is useful for verifying that the Jellyfin notifier is correctly configured. This aligns with the functionality provided for other notifiers and is a good practice for user experience.- 45-74: The
update_librarymethod correctly handles updating the Jellyfin library. It supports updating the library for a specific show or performing a full library refresh. The use ofurljointo construct the URL is appropriate and prevents errors in URL construction. However, ensure thatsettings.JELLYFIN_HOSTis properly sanitized and validated elsewhere in the code to prevent injection attacks.Verification successful
The verification process revealed that
JELLYFIN_HOSTis processed throughconfig.clean_url(jellyfin_host)insickchill/views/config/notifications.pyandcheck_setting_str(settings.CFG, "Jellyfin", "jellyfin_host")insickchill/start.py. These instances suggest mechanisms for sanitizing and validatingJELLYFIN_HOST, addressing the concern raised in the review comment about ensuring proper sanitization and validation to prevent injection attacks. Therefore, the review comment is consistent with the codebase as it correctly identifies the importance of sanitization and validation, which are indeed implemented.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that settings.JELLYFIN_HOST is sanitized and validated grep -R "JELLYFIN_HOST" sickchill/ | grep "sanitize"Length of output: 50
Script:
#!/bin/bash # Broaden the search to find validation or sanitization of JELLYFIN_HOST # Look for common validation or sanitization patterns, including the use of "validate", "check", or direct usage contexts grep -R "JELLYFIN_HOST" sickchill/ | grep -E "validate|check|clean"Length of output: 285
sickchill/oldbeard/stevedore.py (1)
- 89-89: Adding
"jellyfin = oldbeard.notifiers.jellyfin:Notifier"to thenotifiersextension manager is the correct way to register the Jellyfin notifier. This allows SickChill to dynamically discover and use the Jellyfin notifier alongside other notifiers. The change is consistent with the registration of other notifiers and follows best practices for extensibility.sickchill/settings.py (1)
- 535-535: The
USE_JELLYFINsetting is a straightforward boolean flag to enable or disable Jellyfin integration. This approach is consistent with the existing pattern for other services in the application.Ensure that the application logic correctly checks this flag before attempting to use Jellyfin-related functionality.
sickchill/views/config/notifications.py (2)
- 60-62: The addition of
use_jellyfin,jellyfin_host, andjellyfin_apikeyparameters to thesaveNotificationsmethod signature correctly aligns with the PR's objective to integrate Jellyfin as a notifier option. This change allows the method to accept Jellyfin-specific settings from the user interface.- 276-278: The logic to set Jellyfin settings based on the provided parameters is correctly implemented. Using
config.checkbox_to_valueforuse_jellyfinensures that the boolean setting is correctly interpreted from the checkbox input. Similarly,config.clean_urlforjellyfin_hostandfilters.unhideforjellyfin_apikeyare appropriate for sanitizing and securely handling these inputs. This ensures that the Jellyfin settings are correctly updated in SickChill's configuration.sickchill/oldbeard/show_queue.py (1)
- 749-751: The addition of Jellyfin notifier functionality is correctly implemented and follows the pattern used by other notifiers such as Kodi, Plex, and Emby. This ensures consistency in the way SickChill handles library updates across different media servers. However, it's important to ensure that the
jellyfin_notifierobject is properly initialized and available in thenotifiersmodule, as this code assumes its presence and readiness to use.Ensure that the
jellyfin_notifierobject is correctly set up in thenotifiersmodule and that it has a methodupdate_librarythat accepts ashowobject as its parameter. This is crucial for the integration to work as expected without causing runtime errors.sickchill/gui/slick/views/layouts/main.mako (1)
- 204-206: The conditional block for displaying the "Update Jellyfin" link is correctly implemented and follows the pattern used for other services like PLEX and KODI. This ensures consistency across the application and maintains a clean user interface by only showing relevant options.
However, for future maintainability, consider abstracting the condition checks for service configuration (like
settings.USE_JELLYFIN,settings.JELLYFIN_HOST, andsettings.JELLYFIN_APIKEY) into a helper function or method. This approach can simplify the template code and centralize the logic for determining if a service is configured, making it easier to update or extend in the future.sickchill/oldbeard/postProcessor.py (3)
- 1209-1211: The addition of Jellyfin library update functionality is correctly implemented by calling
notifiers.jellyfin_notifier.update_library(episode_object.show)within theprocessmethod. This ensures that the Jellyfin library is updated whenever a new episode is processed, aligning with the behavior for other notifiers like Kodi, Plex, and Emby. The method call is appropriately placed within the try-except block that handles notification and library update operations, ensuring that any exceptions raised by the Jellyfin notifier do not halt the post-processing workflow.- 1209-1211: The
guessit_finditfunction provides an alternative way to parse file names using theguessitlibrary, falling back to the traditionalNameParserif necessary. This approach enhances the flexibility and accuracy of parsing, especially for complex or non-standard file names. However, ensure that theguessitlibrary's behavior aligns closely with SickChill's requirements for parsing show names and episodes. It might be beneficial to add more detailed logging or error handling around theguessitparsing to help diagnose parsing issues.- 1206-1214: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1211]
Throughout the file, there are several instances where string formatting could be modernized to use f-strings, introduced in Python 3.6. F-strings provide a more readable and concise way to format strings compared to the older
.format()method. For example, replace_("File {directory} doesn't exist, did unrar fail?").format(directory=self.directory)withf"File {self.directory} doesn't exist, did unrar fail?". Consider refactoring these instances to use f-strings for improved readability.sickchill/gui/slick/css/style.css (2)
- 349-352: The CSS for
.menu-icon-jellyfincorrectly sets the background image and size for the Jellyfin menu icon. This ensures consistency with other notifier icons in the UI.- 2094-2097: The CSS for
.icon-notifiers-jellyfinproperly defines the background image and size for the Jellyfin notifier icon, aligning with the visual style of other notifier icons.sickchill/views/home.py (3)
- 263-266: The method
haveJELLYFINcorrectly checks if Jellyfin is enabled in the settings. This is a straightforward static method that returns a boolean value based on thesettings.USE_JELLYFINflag.- 561-569: The method
testJELLYFINproperly handles the testing of Jellyfin notifications. It correctly retrieves and sanitizes user input before calling thetest_notifymethod on thejellyfin_notifier. This method follows good practices for handling external input and provides clear feedback to the user based on the result of the test notification.- 1511-1526: The method
updateJELLYFINis implemented to trigger an update in the Jellyfin library, optionally for a specific show. It correctly checks for the presence of a show object and calls theupdate_librarymethod on thejellyfin_notifier. The method also handles user feedback appropriately by displaying a notification message based on the outcome of the update operation.sickchill/start.py (2)
- 58-58: Adding Jellyfin to the configuration sections is a necessary step for integrating Jellyfin as a notifier option. This change correctly follows the pattern established by other notifiers like Emby, Growl, etc.
- 1371-1375: The configuration for Jellyfin is correctly added to the
save_configfunction, ensuring that Jellyfin settings are persisted across application restarts. This aligns with the handling of other notifier configurations.sickchill/gui/slick/views/config_notifications.mako (1)
- 560-621: The Jellyfin integration section has been added correctly, following the structure and conventions used for other services in the file. The use of Mako template syntax, HTML form elements, and SickChill settings variables is consistent with the rest of the file. The inclusion of a test button (
id="testJELLYFIN") and the use of thecheckedfunction to reflect the current configuration state are best practices for user interface design in this context.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
sickchill/gui/slick/js/core.min.jsis excluded by:!**/*.min.js
Files selected for processing (2)
- sickchill/oldbeard/notifiers/init.py (2 hunks)
- sickchill/views/home.py (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- sickchill/oldbeard/notifiers/init.py
- sickchill/views/home.py
|
Man coderabbit can be a pain sometimes lol. Thanks for this. |
Feature Request
Proposed changes in this pull request:
Basically a clone of the emby plugin before recent changes. Still uses the
X-Emby-Tokenfor auth.Only minor changes are update the uri's to remove
embySwitch endpoints to use
postrequestsFixed the test notification button for jellyfin users. (I never see a message in the jellyfin ui, but the api server responds with 204 ok)
API Endpoint docs
https://api.jellyfin.org/#tag/Notifications/operation/CreateAdminNotification (Used when testing connectivity)
https://api.jellyfin.org/#tag/Library/operation/PostAddedSeries (Used for post processing, updates specific show using tvdbid)
https://api.jellyfin.org/#tag/Library/operation/RefreshLibrary ( Used by Manage -> Update Jellyfin button)
Config
Update full library
Update single show
Summary by CodeRabbit