From 86a0fb04002164c098c4ccb9d8af898de83338a1 Mon Sep 17 00:00:00 2001 From: Jesse Bannon Date: Fri, 28 Jul 2023 00:04:39 -0700 Subject: [PATCH] refactor --- src/ytdl_sub/downloaders/url/downloader.py | 4 +- src/ytdl_sub/entries/entry.py | 6 +-- src/ytdl_sub/plugins/embed_thumbnail.py | 11 ++--- src/ytdl_sub/plugins/music_tags.py | 2 +- src/ytdl_sub/plugins/split_by_chapters.py | 2 +- .../subscriptions/subscription_download.py | 10 ++++- src/ytdl_sub/utils/thumbnail.py | 2 +- tests/e2e/youtube/test_video.py | 42 +++++++++++++++++++ .../youtube/test_video_missing_thumb.json | 5 +++ .../youtube/test_video_missing_thumb.txt | 14 +++++++ 10 files changed, 84 insertions(+), 14 deletions(-) create mode 100644 tests/resources/expected_downloads_summaries/youtube/test_video_missing_thumb.json create mode 100644 tests/resources/transaction_log_summaries/youtube/test_video_missing_thumb.txt diff --git a/src/ytdl_sub/downloaders/url/downloader.py b/src/ytdl_sub/downloaders/url/downloader.py index 2ea22f2e..5cbadc4f 100644 --- a/src/ytdl_sub/downloaders/url/downloader.py +++ b/src/ytdl_sub/downloaders/url/downloader.py @@ -84,7 +84,7 @@ class UrlDownloaderThumbnailPlugin(SourcePluginExtension): if thumbnail_id == ThumbnailTypes.LATEST_ENTRY: # always save in dry-run even if it doesn't exist... - if self.is_dry_run or entry.is_thumbnail_available(): + if self.is_dry_run or entry.is_thumbnail_downloaded(): self.save_file( file_name=entry.get_download_thumbnail_name(), output_file_name=thumbnail_name, @@ -350,7 +350,7 @@ class MultiUrlDownloader(SourcePlugin[MultiUrlValidator]): is_downloaded_fn=None if self.is_dry_run else entry.is_downloaded, is_thumbnail_downloaded_fn=None if (self.is_dry_run or not self.is_entry_thumbnails_enabled) - else entry.is_thumbnail_downloaded, + else entry.is_thumbnail_downloaded_via_ytdlp, url=entry.webpage_url, ) return Entry(download_entry_dict, working_directory=self.working_directory) diff --git a/src/ytdl_sub/entries/entry.py b/src/ytdl_sub/entries/entry.py index 2d19f2fc..2432b85d 100644 --- a/src/ytdl_sub/entries/entry.py +++ b/src/ytdl_sub/entries/entry.py @@ -84,16 +84,16 @@ class Entry(EntryVariables, BaseEntry): file.write(kwargs_json) @final - def is_thumbnail_downloaded(self) -> bool: + def is_thumbnail_downloaded_via_ytdlp(self) -> bool: """ Returns ------- - True if the thumbnail file exist locally. False otherwise. + True if ANY thumbnail file exist locally. False otherwise. """ return self.get_ytdlp_download_thumbnail_path() is not None @final - def is_thumbnail_available(self) -> bool: + def is_thumbnail_downloaded(self) -> bool: """ Returns ------- diff --git a/src/ytdl_sub/plugins/embed_thumbnail.py b/src/ytdl_sub/plugins/embed_thumbnail.py index 512f7790..4886f707 100644 --- a/src/ytdl_sub/plugins/embed_thumbnail.py +++ b/src/ytdl_sub/plugins/embed_thumbnail.py @@ -89,12 +89,13 @@ class EmbedThumbnailPlugin(Plugin[EmbedThumbnailOptions]): logger.warning("webm does not support embedded thumbnails, skipping") return None - if not entry.is_thumbnail_downloaded(): - logger.warning( - "Cannot embed thumbnail for '%s' because it is not available", entry.title - ) - if not self.is_dry_run: + if not entry.is_thumbnail_downloaded(): + logger.warning( + "Cannot embed thumbnail for '%s' because it is not available", entry.title + ) + return None + if entry.ext in AUDIO_CODEC_EXTS: self._embed_audio_file(entry) else: diff --git a/src/ytdl_sub/plugins/music_tags.py b/src/ytdl_sub/plugins/music_tags.py index 955591b6..71e4255e 100644 --- a/src/ytdl_sub/plugins/music_tags.py +++ b/src/ytdl_sub/plugins/music_tags.py @@ -161,7 +161,7 @@ class MusicTagsPlugin(Plugin[MusicTagsOptions]): ) setattr(audio_file, tag_name, tag_value[0]) - if self.plugin_options.embed_thumbnail and entry.is_thumbnail_available(): + if self.plugin_options.embed_thumbnail and entry.is_thumbnail_downloaded(): with open(entry.get_download_thumbnail_path(), "rb") as thumb: mediafile_img = mediafile.Image( data=thumb.read(), desc="cover", type=mediafile.ImageType.front diff --git a/src/ytdl_sub/plugins/split_by_chapters.py b/src/ytdl_sub/plugins/split_by_chapters.py index f6eef699..50717854 100644 --- a/src/ytdl_sub/plugins/split_by_chapters.py +++ b/src/ytdl_sub/plugins/split_by_chapters.py @@ -202,7 +202,7 @@ class SplitByChaptersPlugin(SplitPlugin[SplitByChaptersOptions]): # Copy the original vid thumbnail to the working directory with the new uid. This so # downstream logic thinks this split video has its own thumbnail - if entry.is_thumbnail_available(): + if entry.is_thumbnail_downloaded(): FileHandler.copy( src_file_path=entry.get_download_thumbnail_path(), dst_file_path=Path(self.working_directory) diff --git a/src/ytdl_sub/subscriptions/subscription_download.py b/src/ytdl_sub/subscriptions/subscription_download.py index 97a96019..5e04682d 100644 --- a/src/ytdl_sub/subscriptions/subscription_download.py +++ b/src/ytdl_sub/subscriptions/subscription_download.py @@ -1,4 +1,5 @@ import contextlib +import logging import os import shutil from abc import ABC @@ -21,6 +22,9 @@ from ytdl_sub.utils.exceptions import ValidationException from ytdl_sub.utils.file_handler import FileHandler from ytdl_sub.utils.file_handler import FileHandlerTransactionLog from ytdl_sub.utils.file_handler import FileMetadata +from ytdl_sub.utils.logger import Logger + +logger: logging.Logger = Logger.get() def _get_split_plugin(plugins: List[Plugin]) -> Optional[SplitPlugin]: @@ -68,7 +72,7 @@ class SubscriptionDownload(BaseSubscription, ABC): entry=entry, ) - if self.output_options.thumbnail_name and entry.is_thumbnail_available(): + if self.output_options.thumbnail_name and entry.is_thumbnail_downloaded(): output_thumbnail_name = self.overrides.apply_formatter( formatter=self.output_options.thumbnail_name, entry=entry ) @@ -80,6 +84,10 @@ class SubscriptionDownload(BaseSubscription, ABC): entry=entry, copy_file=True, ) + elif not entry.is_thumbnail_downloaded(): + logger.warning( + "Cannot save thumbnail for '%s' because it is not available", entry.title + ) if self.output_options.info_json_name: output_info_json_name = self.overrides.apply_formatter( diff --git a/src/ytdl_sub/utils/thumbnail.py b/src/ytdl_sub/utils/thumbnail.py index a9aa5243..0f87a56a 100644 --- a/src/ytdl_sub/utils/thumbnail.py +++ b/src/ytdl_sub/utils/thumbnail.py @@ -33,7 +33,7 @@ def try_convert_download_thumbnail(entry: Entry) -> None: download_thumbnail_path_as_jpg = entry.get_download_thumbnail_path() # If it was already converted, do not convert again - if os.path.isfile(download_thumbnail_path_as_jpg): + if entry.is_thumbnail_downloaded(): return if not download_thumbnail_path: diff --git a/tests/e2e/youtube/test_video.py b/tests/e2e/youtube/test_video.py index 63b9c220..9f14d400 100644 --- a/tests/e2e/youtube/test_video.py +++ b/tests/e2e/youtube/test_video.py @@ -1,10 +1,15 @@ +from unittest.mock import patch + import pytest from conftest import preset_dict_to_dl_args from e2e.conftest import mock_run_from_cli from expected_download import assert_expected_downloads from expected_transaction_log import assert_transaction_log_matches +from ytdl_sub.downloaders.ytdlp import YTDLP +from ytdl_sub.entries.entry import Entry from ytdl_sub.subscriptions.subscription import Subscription +from ytdl_sub.utils.file_handler import FileHandler @pytest.fixture @@ -140,6 +145,43 @@ class TestYoutubeVideo: expected_download_summary_file_name="youtube/test_video.json", ) + def test_single_video_download_missing_thumbnail( + self, + music_video_config, + single_video_preset_dict, + working_directory, + output_directory, + ): + single_video_subscription = Subscription.from_dict( + config=music_video_config, + preset_name="music_video_single_video_test", + preset_dict=single_video_preset_dict, + ) + + def delete_entry_thumb(entry: Entry) -> None: + FileHandler.delete(entry.get_download_thumbnail_path()) + + # Pretend the thumbnail did not download via returning nothing for its downloaded path + with patch.object(YTDLP, "_EXTRACT_ENTRY_NUM_RETRIES", 1), patch.object( + Entry, "get_ytdlp_download_thumbnail_path" + ) as mock_ytdlp_path, patch( + "ytdl_sub.downloaders.url.downloader.try_convert_download_thumbnail", + side_effect=delete_entry_thumb, + ): + mock_ytdlp_path.return_value = None + transaction_log = single_video_subscription.download(dry_run=False) + + assert_transaction_log_matches( + output_directory=output_directory, + transaction_log=transaction_log, + transaction_log_summary_file_name="youtube/test_video_missing_thumb.txt", + ) + assert_expected_downloads( + output_directory=output_directory, + dry_run=False, + expected_download_summary_file_name="youtube/test_video_missing_thumb.json", + ) + @pytest.mark.parametrize("dry_run", [True, False]) def test_single_video_download_from_cli_dl( self, diff --git a/tests/resources/expected_downloads_summaries/youtube/test_video_missing_thumb.json b/tests/resources/expected_downloads_summaries/youtube/test_video_missing_thumb.json new file mode 100644 index 00000000..a1f618e7 --- /dev/null +++ b/tests/resources/expected_downloads_summaries/youtube/test_video_missing_thumb.json @@ -0,0 +1,5 @@ +{ + "JMC/JMC - Oblivion Mod ＂Falcor＂ p.1.info.json": "4b361d0ab0220407731553dbe36f12a1", + "JMC/JMC - Oblivion Mod ＂Falcor＂ p.1.mp4": "3744c49f2e447bd7712a5aad5ed36be2", + "JMC/JMC - Oblivion Mod ＂Falcor＂ p.1.nfo": "24cc4e17d2bebc89b2759ce5471d403e" +} \ No newline at end of file diff --git a/tests/resources/transaction_log_summaries/youtube/test_video_missing_thumb.txt b/tests/resources/transaction_log_summaries/youtube/test_video_missing_thumb.txt new file mode 100644 index 00000000..f41464dd --- /dev/null +++ b/tests/resources/transaction_log_summaries/youtube/test_video_missing_thumb.txt @@ -0,0 +1,14 @@ +Files created: +---------------------------------------- +{output_directory}/JMC + JMC - Oblivion Mod ＂Falcor＂ p.1.info.json + JMC - Oblivion Mod ＂Falcor＂ p.1.mp4 + Video Tags: + title: Oblivion Mod "Falcor" p.1 + JMC - Oblivion Mod ＂Falcor＂ p.1.nfo + NFO tags: + musicvideo: + album: Music Videos + artist: JMC + title: Oblivion Mod "Falcor" p.1 + year: 2010 \ No newline at end of file