Providers cleanup #8604
Providers cleanup #8604
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
use has_option rather than hasattr clean up mako, avoid continue if we can, filter the lists in python add ability to only get enabled provider types from sorted_provider_lsits add method to filter show list to remove shows that are being deleted Signed-off-by: miigotu <[email protected]>
Signed-off-by: miigotu <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Signed-off-by: miigotu <[email protected]>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (17)
- qodana.yaml (1 hunks)
- sickchill/gui/slick/views/addShows_popularShows.mako (2 hunks)
- sickchill/gui/slick/views/config_providers.mako (12 hunks)
- sickchill/gui/slick/views/home.mako (2 hunks)
- sickchill/gui/slick/views/home_massAddTable.mako (1 hunks)
- sickchill/gui/slick/views/inc_home_show_list_poster.mako (3 hunks)
- sickchill/oldbeard/config.py (3 hunks)
- sickchill/oldbeard/filters.py (2 hunks)
- sickchill/oldbeard/providers/init.py (2 hunks)
- sickchill/oldbeard/providers/eztv.py (1 hunks)
- sickchill/oldbeard/providers/newznab.py (7 hunks)
- sickchill/providers/GenericProvider.py (1 hunks)
- sickchill/show/History.py (9 hunks)
- sickchill/start.py (2 hunks)
- sickchill/tv.py (1 hunks)
- sickchill/views/config/providers.py (3 hunks)
- sickchill/views/manage/add_shows.py (1 hunks)
Files skipped from review due to trivial changes (11)
- qodana.yaml
- sickchill/gui/slick/views/addShows_popularShows.mako
- sickchill/gui/slick/views/config_providers.mako
- sickchill/gui/slick/views/home.mako
- sickchill/gui/slick/views/home_massAddTable.mako
- sickchill/gui/slick/views/inc_home_show_list_poster.mako
- sickchill/oldbeard/config.py
- sickchill/oldbeard/providers/eztv.py
- sickchill/oldbeard/providers/newznab.py
- sickchill/tv.py
- sickchill/views/config/providers.py
Additional comments: 15
sickchill/providers/GenericProvider.py (1)
- 606-640: The
check_and_update_urlsmethod has been refactored to improve maintainability. It checks for the existence ofcustom_urland the presence ofurlandurlsattributes. It also introduces thevalid_urlmethod to validate thecustom_url. Ifcustom_urlis present and valid, it stores the originalurlandurlsin case they are reset later. It then updates each URL inurlsto have the correct proxy URL based on thecustom_url. Ifcustom_urlis removed or reset, it restores the originalurlandurls. The method now returns a boolean indicating whether the update was successful. This is a good practice as it makes the code more readable and maintainable.sickchill/oldbeard/filters.py (1)
- 1-1: The import statement is fine as long as the
settingsmodule is used in the code.sickchill/oldbeard/providers/__init__.py (2)
132-141: The function
sorted_provider_listhas been updated to accept two new optional parameters:randomizeandonly_enabled. The initial list of providers is created by combiningsettings.providerList,settings.newznab_provider_list, andsettings.torrent_rss_provider_list. A dictionaryprovider_dictis created with the provider's id as the key and the provider object as the value. An empty listnew_provider_listis initialized to store the sorted list of providers.157-162: The
only_enabledparameter is used to filter thenew_provider_listto include only enabled providers. Therandomizeparameter is used to shuffle thenew_provider_listif set toTrue.sickchill/views/manage/add_shows.py (1)
- 314-332: The logic for filtering out shows that are already present in the
settings.showListbased on their IMDb IDs is correct. However, the use of set comprehension here might be a bit confusing. It might be more readable to use a traditional for loop and an if condition to filter out the shows. Also, the default values for 'rating' and 'votes' are being set correctly.Here's a more readable version of the same logic:
filtered_popular_shows = [] for show in popular_shows: if show.getID() and show.getID().strip("tt") not in [show.imdb_id.strip("tt") for show in settings.showList if show.imdb_id]: show.setdefault('rating', '0.0') show.setdefault('votes', '0') filtered_popular_shows.append(show) popular_shows = filtered_popular_showssickchill/start.py (2)
- 776-834: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [778-856]
The change from
hasattrtohas_optionis a good one. It makes the code more explicit and easier to understand. It also reduces the risk of accessing an attribute that doesn't exist. However, ensure that thehas_optionmethod is implemented in all provider classes and it correctly checks for the existence of the option.
- 1092-1146: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [1094-1168]
The changes in this hunk are similar to the previous one. The use of
has_optionmethod improves the code readability and reduces the risk of accessing an attribute that doesn't exist. Again, ensure that thehas_optionmethod is implemented in all provider classes and it correctly checks for the existence of the option.sickchill/show/History.py (8)
12-15: The import of
GenericProvideris new. Ensure that it is used correctly in the code.36-47: The SQL query in the
removefunction has been modified to use parameterized queries, which is a good practice to prevent SQL injection. The parameters are correctly added to theparamslist.77-83: The SQL query in the
getfunction has been modified to use parameterized queries, which is a good practice to prevent SQL injection. The placeholders are correctly added to the query.133-139: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [123-139]
The
_logHistoryItemfunction has been renamed to_log_history_itemto follow PEP 8 naming conventions. The function now uses parameterized queries, which is a good practice to prevent SQL injection. The parameters are correctly added to the query.
145-159: The
log_snatchfunction has been updated to use theGenericProviderclass. Ensure that theprovider_classis correctly used in the function.191-191: The
_log_history_itemfunction call in thelog_downloadfunction has been modified to pass thegroupparameter asgroup or -1. This ensures that a default value is used ifgroupisNone.199-212: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [193-211]
The
log_subtitlefunction has been updated to include thescoresparameter. The_log_history_itemfunction call in thelog_subtitlefunction has been modified to pass thesubtitle.provider_nameparameter as theproviderargument. Ensure that thescoresparameter is correctly used in the function.
- 220-225: The
_log_history_itemfunction call in thelog_failedfunction has been modified to use parameterized queries, which is a good practice to prevent SQL injection. The parameters are correctly added to the query.
sickchill/oldbeard/filters.py
Outdated
| def filter_shows_being_removed(show_list): | ||
| return {show for show in show_list if not (settings.showQueueScheduler.action.is_in_remove_queue(show) or settings.showQueueScheduler.action.is_being_removed(show))} |
There was a problem hiding this comment.
The filter_shows_being_removed function is well written and follows the set comprehension syntax. It filters out shows that are either in the remove queue or are being removed. However, it's a good practice to add a docstring to explain what the function does.
+ """
+ Filters out shows that are either in the remove queue or are being removed.
+
+ :param show_list: List of shows to filter
+ :return: Set of shows not in the remove queue or being removed
+ """
def filter_shows_being_removed(show_list):
return {show for show in show_list if not (settings.showQueueScheduler.action.is_in_remove_queue(show) or settings.showQueueScheduler.action.is_being_removed(show))}Commitable suggestion (Beta)
| def filter_shows_being_removed(show_list): | |
| return {show for show in show_list if not (settings.showQueueScheduler.action.is_in_remove_queue(show) or settings.showQueueScheduler.action.is_being_removed(show))} | |
| def filter_shows_being_removed(show_list): | |
| """ | |
| Filters out shows that are either in the remove queue or are being removed. | |
| :param show_list: List of shows to filter | |
| :return: Set of shows not in the remove queue or being removed | |
| """ | |
| return {show for show in show_list if not (settings.showQueueScheduler.action.is_in_remove_queue(show) or settings.showQueueScheduler.action.is_being_removed(show))} |
|
|
||
| # add all enabled providers first | ||
| for current_module in provider_dict: | ||
| if provider_dict[current_module] not in new_provider_list and provider_dict[current_module].is_enabled: | ||
| if current_module not in new_provider_list and provider_dict[current_module].is_enabled: | ||
| new_provider_list.append(provider_dict[current_module]) | ||
|
|
||
| # add any modules that are missing from that list | ||
| for current_module in provider_dict: | ||
| if provider_dict[current_module] not in new_provider_list: | ||
| new_provider_list.append(provider_dict[current_module]) | ||
|
|
There was a problem hiding this comment.
The logic for adding enabled providers to the new_provider_list and then adding any missing modules remains the same. However, the check current_module not in new_provider_list seems redundant because provider_dict is a dictionary where each key is unique. Therefore, the condition will always be true. This redundancy can be removed to simplify the code.
- if current_module not in new_provider_list and provider_dict[current_module].is_enabled:
+ if provider_dict[current_module].is_enabled:
- if provider_dict[current_module] not in new_provider_list:
+ new_provider_list.append(provider_dict[current_module])Commitable suggestion (Beta)
| # add all enabled providers first | |
| for current_module in provider_dict: | |
| if provider_dict[current_module] not in new_provider_list and provider_dict[current_module].is_enabled: | |
| if current_module not in new_provider_list and provider_dict[current_module].is_enabled: | |
| new_provider_list.append(provider_dict[current_module]) | |
| # add any modules that are missing from that list | |
| for current_module in provider_dict: | |
| if provider_dict[current_module] not in new_provider_list: | |
| new_provider_list.append(provider_dict[current_module]) | |
| # add all enabled providers first | |
| for current_module in provider_dict: | |
| if provider_dict[current_module].is_enabled: | |
| new_provider_list.append(provider_dict[current_module]) | |
| # add any modules that are missing from that list | |
| for current_module in provider_dict: | |
| new_provider_list.append(provider_dict[current_module]) |
WalkthroughThe changes primarily focus on improving the handling of providers in SickChill, a Python-based automatic Video Library Manager. The updates include refactoring code for better readability, renaming variables for clarity, enhancing search methods, and updating the user interface for provider configuration. Security is also improved with the use of parameterized SQL queries. Changes
TipsChat with CodeRabbit Bot (
|
Signed-off-by: miigotu <[email protected]>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sickchill/oldbeard/providers/init.py (1 hunks)
- sickchill/tv.py (1 hunks)
Files skipped from review due to trivial changes (1)
- sickchill/tv.py
Additional comments: 2
sickchill/oldbeard/providers/__init__.py (2)
134-162: The function
sorted_provider_listhas been updated to include two optional parameters:randomizeandonly_enabled. The function now returns a list of providers sorted based on thesettings.PROVIDER_ORDERlist, with enabled providers added first. Ifonly_enabledisTrue, the function filters out disabled providers. IfrandomizeisTrue, the function shuffles the list before returning it. This function is more flexible and can be used in different contexts. However, ensure that all calls to this function throughout the codebase have been updated to match the new signature.161-162: The
shufflefunction is now called on thenew_provider_listifrandomizeisTrue. This is a good use of therandomizeparameter and should work as expected.
Signed-off-by: miigotu <[email protected]>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 12
Configuration used: CodeRabbit UI
Files selected for processing (5)
- sickchill/gui/slick/views/config_providers.mako (12 hunks)
- sickchill/oldbeard/filters.py (2 hunks)
- sickchill/oldbeard/providers/init.py (1 hunks)
- sickchill/providers/GenericProvider.py (5 hunks)
- sickchill/views/manage/add_shows.py (1 hunks)
Files not summarized due to errors (1)
- sickchill/gui/slick/views/config_providers.mako: Error: Message exceeds token limit
Additional comments: 9
sickchill/oldbeard/filters.py (1)
- 1-1: The import statement is fine. Ensure that the
settingsmodule is available in the context where this file is used.sickchill/providers/GenericProvider.py (3)
387-390: The
make_idmethod has been updated to replace non-alphanumeric characters with underscores. This is a good practice for creating IDs that are URL safe and filesystem safe.499-505: The regular expression in the
hash_from_magnetmethod has been updated to match the torrent hash. Ensure that this new regular expression correctly matches the intended patterns.606-640: The
check_and_update_urlsmethod has been refactored for improved readability and maintainability. It now checks if a custom URL is set and valid, and if so, updates theurlsdictionary accordingly. If the custom URL is not set or invalid, it restores the original URLs. This is a good practice for handling custom URLs.sickchill/gui/slick/views/config_providers.mako (5)
21-27: Ensure that the
addProviderandaddTorrentRssProviderfunctions properly handle special characters in the provider's name, URL, key, categories, and cookies to prevent potential XSS attacks. [SECURITY]76-91: The provider's URL is being used in an anchor tag. Make sure to escape it to prevent potential XSS attacks. [SECURITY]
141-146: The provider's URL is being displayed in an input field. Make sure to escape it to prevent potential XSS attacks. [SECURITY]
155-155: The provider's key is being displayed in an input field. Make sure to escape it to prevent potential XSS attacks. [SECURITY]
421-421: The provider's username, password, API key, passkey, and cookies are being displayed in input fields. Make sure to escape these values to prevent potential XSS attacks. [SECURITY]
| def filter_shows_being_removed(show_list): | ||
| return { | ||
| show | ||
| for show in show_list | ||
| if not (settings.showQueueScheduler.action.is_in_remove_queue(show) or settings.showQueueScheduler.action.is_being_removed(show)) | ||
| } |
There was a problem hiding this comment.
The new function filter_shows_being_removed is logically correct. It filters out shows that are in the remove queue or being removed. However, the function could be more readable if the condition inside the if statement is simplified. Consider using a variable to store the condition result.
def filter_shows_being_removed(show_list):
return {
show
for show in show_list
if not is_show_being_removed(show)
}
def is_show_being_removed(show):
action = settings.showQueueScheduler.action
return action.is_in_remove_queue(show) or action.is_being_removed(show)|
|
||
| self.anime_only = False | ||
| self.bt_cache_urls = [ | ||
| # 'http://torcache.net/torrent/{torrent_hash}.torrent', | ||
| # 'http://torrentproject.se/torrent/{torrent_hash}.torrent', | ||
| "http://thetorrent.org/torrent/{torrent_hash}.torrent", | ||
| # 'http://btdig.com/torrent/{torrent_hash}.torrent', | ||
| # "http://torcache.net/torrent/{torrent_hash}.torrent", | ||
| # "http://torrentproject.se/torrent/{torrent_hash}.torrent", | ||
| # "http://thetorrent.org/torrent/{torrent_hash}.torrent", | ||
| # "http://btdig.com/torrent/{torrent_hash}.torrent", | ||
| ("https://torrage.info/download?h={torrent_hash}", "https://torrage.info/torrent.php?h={torrent_hash}"), | ||
| "https://itorrents.org/torrent/{torrent_hash}.torrent?title={torrent_name}", | ||
| ] |
There was a problem hiding this comment.
The commented out URLs in the bt_cache_urls list are not being used. If these URLs are not needed, consider removing them to clean up the code. If they might be used in the future, add a comment explaining why they are commented out.
| t = PageTemplate(rh=self, filename="addShows_popularShows.mako") | ||
| try: | ||
| popular_shows = imdb_popular.fetch_popular_shows() | ||
|
|
||
| popular_shows = { | ||
| show | ||
| for show in popular_shows | ||
| if show.getID() and show.getID().strip("tt") not in {show.imdb_id.strip("tt") for show in settings.showList if show.imdb_id} | ||
| } | ||
| for show in popular_shows: | ||
| show.setdefault("rating", "0.0") | ||
| show.setdefault("votes", "0") | ||
|
|
||
| imdb_exception = None | ||
| except Exception as error: | ||
| logger.warning(f"Could not get popular shows: {error}") |
There was a problem hiding this comment.
The code is filtering out popular shows that are already present in the settings.showList data structure. This is a good practice to avoid duplication. However, the code is using a set comprehension inside a list comprehension which can be a bit hard to read and understand. Consider breaking this down into multiple steps for better readability. Also, the code is setting default values for "rating" and "votes" attributes of each show in the popular_shows list. This is a good practice to avoid potential KeyError exceptions later in the code if these keys are not present in the dictionary. However, it would be better to use a more pythonic way to set default values for dictionary keys using the dict.get(key, default) method.
# Break down the filtering step into multiple lines for better readability
show_ids_in_settings = {show.imdb_id.strip("tt") for show in settings.showList if show.imdb_id}
popular_shows = [show for show in popular_shows if show.getID() and show.getID().strip("tt") not in show_ids_in_settings]
# Use dict.get(key, default) to set default values for dictionary keys
for show in popular_shows:
show["rating"] = show.get("rating", "0.0")
show["votes"] = show.get("votes", "0")| def sorted_provider_list(randomize=False) -> List[Union[TorrentProvider, NZBProvider, TorrentRssProvider, NZBProvider, GenericProvider]]: | ||
| initialList = settings.providerList + settings.newznab_provider_list + settings.torrent_rss_provider_list | ||
| provider_dict = {x.get_id(): x for x in initialList} | ||
| def sorted_provider_list(randomize=False, only_enabled=False) -> List[Union[TorrentProvider, NZBProvider, TorrentRssProvider, NZBProvider, GenericProvider]]: |
There was a problem hiding this comment.
The return type of the function sorted_provider_list is too complex. Consider creating a type alias for the Union type to improve readability.
ProviderType = Union[TorrentProvider, NZBProvider, TorrentRssProvider, NZBProvider, GenericProvider]
def sorted_provider_list(randomize=False, only_enabled=False) -> List[ProviderType]:| initialList = settings.providerList + settings.newznab_provider_list + settings.torrent_rss_provider_list | ||
| provider_dict = {x.get_id(): x for x in initialList} | ||
| def sorted_provider_list(randomize=False, only_enabled=False) -> List[Union[TorrentProvider, NZBProvider, TorrentRssProvider, NZBProvider, GenericProvider]]: | ||
| provider_types = List[Union[GenericProvider, TorrentProvider, NZBProvider, TorrentRssProvider]] |
There was a problem hiding this comment.
The variable provider_types is not used anywhere in the function. Consider removing it.
| if randomize: | ||
| shuffle(new_provider_list) |
There was a problem hiding this comment.
The randomize parameter is used to shuffle the new_provider_list. This is a side effect that might not be expected by the caller. Consider removing this parameter and letting the caller shuffle the list if needed.
| @@ -57,7 +57,7 @@ | |||
|
|
|||
| % if not settings.USE_NZBS or not settings.USE_TORRENTS: | |||
| <blockquote style="margin: 20px 0;">NZB/${_('Torrent and NZB providers can be toggled in ')} | |||
| <b><a href="/config/search">Search Settings</a></b></blockquote> | |||
| <b><a class="primary" href="/config/search">Search Settings</a></b></blockquote> | |||
There was a problem hiding this comment.
The link to 'Search Settings' should be generated dynamically to ensure it always points to the correct location, even if the route changes in the future.
| provider_config_list = providers.sorted_provider_list(only_enabled = True) | ||
| %> | ||
| % if provider_config_list: | ||
| % if any(provider.is_enabled for provider in provider_config_list): | ||
| <select id="editAProvider" class="form-control input-sm input250"> | ||
| % for cur_provider in provider_config_list: | ||
| % for cur_provider in (provider for provider in provider_config_list if provider.is_enabled): | ||
| <option value="${cur_provider.get_id()}">${cur_provider.name}</option> | ||
| % endfor | ||
| </select> | ||
| % else: | ||
| <label>${_('no providers available to configure.')}</label> | ||
| <label>${_('No providers available to configure. You must enable either torrent or newznab first to enable providers.')}</label> | ||
| % endif |
There was a problem hiding this comment.
The provider list is being filtered in the template. Consider moving this logic to the backend for better performance and maintainability.
| <input type="checkbox" name="${provider.get_id("_search_fallback")}" id="${provider.get_id("_search_fallback")}" ${checked(provider.search_fallback_enabled)}/> | ||
| <label for="${provider.get_id("_search_fallback")}"> | ||
| ${_('when searching for a complete season depending on search mode you may return no results, this helps by restarting the search using the opposite search mode.')} | ||
| ss</label> |
There was a problem hiding this comment.
There seems to be a typo at the end of the line. Remove the extra 'ss'.
- ${_('when searching for a complete season depending on search mode you may return no results, this helps by restarting the search using the opposite search mode.')}
- ss</label>
+ ${_('when searching for a complete season depending on search mode you may return no results, this helps by restarting the search using the opposite search mode.')}</label>Commitable suggestion (Beta)
| ss</label> | |
| ${_('when searching for a complete season depending on search mode you may return no results, this helps by restarting the search using the opposite search mode.')}</label> |
There was a problem hiding this comment.
@BKSteve I missed this lol, looks like a Ctrl+sssss 🤷♂️🤣
There was a problem hiding this comment.
Looks like humans are talking. I'll be quiet for now. If you need me again, just tag @coderabbitai in a new comment.
There was a problem hiding this comment.
Halloween prep? Or snakes on a plane? 🐍 🐍
The rabbit showing its usefulness!
There was a problem hiding this comment.
But no easy accept box, have to do the changes suggested by ourselves.
As you merged and this issue is still there. patch-1 raised
| </div> | ||
| </div> | ||
| <div class="row"> | ||
| <div class="col-md-12"> | ||
| <label for="${curTorrentProvider.get_id("_cookies")}"> | ||
| <label for="${provider.get_id("_cookies")}"> |
There was a problem hiding this comment.
The example provided for the 'Cookies' field could be confusing for users. Consider providing a more detailed explanation or a link to a guide on how to get these values.
Can't find the error here. I just want to save this in case I can find it but the entire page needs rewritten in the new UI anyway, and the providers need loaded with stevedore, as does notifiers and other items. Possibly something other than stevedore but I am going to work on that and the new UI instead of this for now.
Summary by CodeRabbit
Historyclass to prevent SQL injection.GenericProviderclass.