Fixing error where all Videos of a Channel would be downloaded because upsert_video() did not update the correct record.

app/src/api.js

@@ -251,7 +251,7 @@ export async function downloadChannel(id) {
         toast({
             type: 'error',
             title: 'Cannot Download!',
-            description: 'This channel does not have a download record.  Create one!',
+            description: 'This channel does not have a download record.  Modify the frequency.',
             time: 5000,
         });
     }

modules/videos/conftest.py

@@ -1,6 +1,8 @@
 import json
 import pathlib
 import shutil
+from itertools import zip_longest
+from typing import List
 from uuid import uuid4
 
 import mock
@@ -32,7 +34,7 @@ def simple_channel(test_session, test_directory) -> Channel:
 def channel_factory(test_session, test_directory):
     """Create a random Channel with a directory, but no frequency."""
 
-    def _(source_id: str = None, download_frequency: DownloadFrequency = None, url: str = None, name: str = None):
+    def factory(source_id: str = None, download_frequency: DownloadFrequency = None, url: str = None, name: str = None):
         name = name or str(uuid4())
         directory = test_directory / name
         directory.mkdir()
@@ -47,7 +49,7 @@ def channel_factory(test_session, test_directory):
         test_session.commit()
         return channel
 
-    return _
+    return factory
 
 
 @pytest.fixture
@@ -98,7 +100,8 @@ def video_factory(test_session, test_directory):
                 with_info_json: dict = None, with_poster_ext: str = None, with_caption_file: bool = False):
         title = title or str(uuid4())
         if channel_id:
-            path = test_directory / f'{title}.mp4'
+            channel = test_session.query(Channel).filter_by(id=channel_id).one()
+            path = (channel.directory or test_directory) / f'{title}.mp4'
         else:
             (test_directory / 'videos/NO CHANNEL').mkdir(parents=True, exist_ok=True)
             path = test_directory / f'videos/NO CHANNEL/{title}.mp4'
@@ -186,3 +189,15 @@ def mock_video_process_runner():
     with mock.patch('modules.videos.downloader.VideoDownloader.process_runner') as mock_process_runner:
         mock_process_runner.return_value = (0, {})
         yield mock_process_runner
+
+
+@pytest.fixture
+def assert_video_ids(test_session):
+    """Check that only the expected Videos are in the DB."""
+
+    def checker(expected_video_ids: List[int]):
+        video_ids = [i.id for i in test_session.query(Video).order_by(Video.id)]
+        for id_, expected in zip_longest(video_ids, expected_video_ids):
+            assert id_ == expected, f'Video ids were not as expected: {expected_video_ids=} != {video_ids=}'
+
+    return checker

modules/videos/downloader.py

@@ -81,26 +81,31 @@ class ChannelDownloader(Downloader, ABC):
                 channel = get_channel(source_id=channel_source_id, return_dict=False)
             channel_id = channel.id
 
-        update_channel(channel)
+        try:
+            update_channel(channel)
 
-        domain = extract_domain(download.url)
-        missing_videos = find_all_missing_videos(channel.id)
-        with get_db_session(commit=True) as session:
-            for video_id, source_id, missing_video in missing_videos:
-                url = video_url_resolver(domain, missing_video)
-                if url in DOWNLOAD_MANAGER_CONFIG.skip_urls:
-                    # Don't try to download skipped videos.
-                    continue
-                # Schedule any missing videos for download.
-                download = self.manager.get_or_create_download(url, session)
-                download.downloader = 'video'
+            domain = extract_domain(download.url)
+            missing_videos = find_all_missing_videos(channel.id)
+            with get_db_session(commit=True) as session:
+                for video_id, source_id, missing_video in missing_videos:
+                    url = video_url_resolver(domain, missing_video)
+                    if url in DOWNLOAD_MANAGER_CONFIG.skip_urls:
+                        # Don't try to download skipped videos.
+                        continue
+                    # Schedule any missing videos for download.
+                    download = self.manager.get_or_create_download(url, session)
+                    download.downloader = 'video'
 
-        if PYTEST:
-            self.manager.do_downloads_sync()
-        else:
-            self.manager.start_downloads()
+            if PYTEST:
+                self.manager.do_downloads_sync()
+            else:
+                self.manager.start_downloads()
 
-        return DownloadResult(success=True, location=f'/videos/channel/{channel_id}/video')
+            return DownloadResult(success=True, location=f'/videos/channel/{channel_id}/video')
+        except Exception:
+            logger.warning(f'Failed to update catalog of channel {download.url}')
+            location = f'/videos/channel/{channel_id}/video' if channel_id else None
+            return DownloadResult(success=False, location=location, error=str(traceback.format_exc()))
 
 
 YT_DLP_BIN = which(

modules/videos/lib.py

@@ -715,9 +715,15 @@ def upsert_video(session: Session, video_path: pathlib.Path, channel: Channel =
         raise ValueError(f'Video path is not absolute: {video_path}')
 
     # This function can update or insert a Video.
+    video = None
     if id_:
         video = session.query(Video).filter_by(id=id_).one()
-    else:
+    if not video:
+        # The video may already be in the database, match the source_id to any existing video.
+        _, _, source_id, _ = parse_video_file_name(video_path)
+        if source_id:
+            video = session.query(Video).filter_by(source_id=source_id).one_or_none()
+    if not video:
         video = Video()
         session.add(video)
 

modules/videos/test/test_lib.py

@@ -7,6 +7,7 @@ from unittest import mock
 import pytest
 import sqlalchemy
 
+from modules.videos import lib
 from modules.videos.common import apply_info_json
 from modules.videos.lib import validate_videos, parse_video_file_name, upsert_video
 from modules.videos.models import Video
@@ -218,3 +219,34 @@ def test_video_modification_datetime(test_session, simple_channel, video_file):
         video = upsert_video(test_session, video_file, simple_channel)
     test_session.commit()
     assert video.modification_datetime == expected
+
+
+def test_refresh_channel_videos(test_session, channel_factory, video_factory, video_file, assert_video_ids):
+    assert_video_ids([])
+
+    # Simulate a channel's catalog being updated.
+    channel1 = channel_factory(name='channel1')
+    test_session.add(Video(source_id='source1', channel_id=channel1.id))
+    test_session.add(Video(source_id='source2', channel_id=channel1.id))
+
+    # Add some real files to be found.
+    destination = channel1.directory / f'channel1_20220510_source1_the title.mp4'
+    shutil.copy(PROJECT_DIR / 'test/big_buck_bunny_720p_1mb.mp4', destination)
+    destination = channel1.directory / f'channel1_20220511_source2_the title.mp4'
+    shutil.copy(PROJECT_DIR / 'test/big_buck_bunny_720p_1mb.mp4', destination)
+
+    lib.refresh_channel_videos(channel1)
+    assert_video_ids([1, 2])
+
+    # These video files were deleted.
+    test_session.add(Video(source_id='this should be removed', channel_id=channel1.id, video_path='foo'))
+    test_session.add(Video(source_id='this should also be removed', video_path='bar'))
+    test_session.commit()
+
+    assert_video_ids([1, 2, 3, 4])
+
+    lib.refresh_channel_videos(channel1)
+    assert_video_ids([1, 2, 4])
+
+    lib.refresh_videos()
+    assert_video_ids([1, 2])

wrolpi/downloader.py

@@ -574,7 +574,8 @@ class DownloadManager:
                     last_successful_download,
                     next_download,
                     status,
-                    url
+                    url,
+                    location
                 FROM download
                 WHERE frequency IS NOT NULL
                 ORDER BY