From 47044cdd1fbb62b5f19ca3f32d607c0eaa64caeb Mon Sep 17 00:00:00 2001 From: Roland Date: Sat, 6 Dec 2025 09:27:40 -0700 Subject: [PATCH] Ensuring domain config saves when tagging/untagging a domain. --- app/src/components/Archive.js | 1 + app/src/components/Channels.js | 1 + .../collections/CollectionTagModal.js | 56 +++-- .../collections/CollectionTagModal.test.js | 235 ++++++++++++++++++ modules/archive/lib.py | 11 +- modules/archive/test/test_domain_tagging.py | 106 +++++--- modules/videos/channel/lib.py | 2 + modules/videos/channel/test/test_lib.py | 53 ++++ wrolpi/collections/lib.py | 58 +++-- wrolpi/collections/models.py | 35 +-- wrolpi/collections/test/test_api.py | 70 ++++++ .../test/test_collection_tagging.py | 76 ++++++ wrolpi/common.py | 4 +- wrolpi/contexts.py | 8 + wrolpi/db.py | 4 +- wrolpi/files/test/test_ebooks.py | 3 +- wrolpi/test/test_contexts.py | 42 ++++ 17 files changed, 651 insertions(+), 114 deletions(-) create mode 100644 wrolpi/test/test_contexts.py diff --git a/app/src/components/Archive.js b/app/src/components/Archive.js index e5839dde..1373534f 100644 --- a/app/src/components/Archive.js +++ b/app/src/components/Archive.js @@ -597,6 +597,7 @@ export function DomainEditPage() { getTagInfo={handleGetTagInfo} onSave={handleTagSave} collectionName="Domain" + hasDirectory={!!domain?.directory} /> {/* Downloads Segment */} diff --git a/app/src/components/Channels.js b/app/src/components/Channels.js index 2e02b929..476e8beb 100644 --- a/app/src/components/Channels.js +++ b/app/src/components/Channels.js @@ -280,6 +280,7 @@ export function ChannelEditPage() { getTagInfo={handleGetTagInfo} onSave={handleTagSave} collectionName="Channel" + hasDirectory={!!channel.directory} /> {/* Downloads Segment */} diff --git a/app/src/components/collections/CollectionTagModal.js b/app/src/components/collections/CollectionTagModal.js index e1e37108..4cfd46db 100644 --- a/app/src/components/collections/CollectionTagModal.js +++ b/app/src/components/collections/CollectionTagModal.js @@ -16,6 +16,8 @@ import {APIButton, Toggle} from '../Common'; * @param {Function} getTagInfo - Async function to fetch tag info: (tagName) => Promise<{suggested_directory, conflict, conflict_message}> * @param {Function} onSave - Async function called when saving: (tagName, directory) => Promise * @param {string} collectionName - Name of the collection for toast messages (e.g., "Domain", "Channel") + * @param {boolean} hasDirectory - Whether the collection has a directory (default true for backward compatibility) + * When false, hides directory toggle and input, always passes null for directory */ export function CollectionTagModal({ open, @@ -25,6 +27,7 @@ export function CollectionTagModal({ getTagInfo, onSave, collectionName = 'Collection', + hasDirectory = true, }) { const [newTagName, setNewTagName] = useState(currentTagName || null); const [moveToTagDirectory, setMoveToTagDirectory] = useState(true); @@ -86,12 +89,15 @@ export function CollectionTagModal({ // Handle save const handleSave = useCallback(async () => { - await onSave(newTagName, moveToTagDirectory ? newTagDirectory : null); + // When hasDirectory is false, always pass null for directory + const directoryToSave = hasDirectory && moveToTagDirectory ? newTagDirectory : null; + await onSave(newTagName, directoryToSave); handleClose(); - }, [newTagName, moveToTagDirectory, newTagDirectory, onSave, handleClose]); + }, [newTagName, hasDirectory, moveToTagDirectory, newTagDirectory, onSave, handleClose]); const modalTitle = currentTagName ? 'Modify Tag' : 'Add Tag'; - const saveButtonText = moveToTagDirectory ? 'Move' : 'Save'; + // Show "Move" button only when hasDirectory is true and move toggle is on + const saveButtonText = hasDirectory && moveToTagDirectory ? 'Move' : 'Save'; return ( - - - - - - - - setNewTagDirectory(value)} - disabled={!moveToTagDirectory} - /> - - - {conflictMessage && ( + {hasDirectory && ( + + + + + + )} + {hasDirectory && ( + + + setNewTagDirectory(value)} + disabled={!moveToTagDirectory} + /> + + + )} + {hasDirectory && conflictMessage && ( diff --git a/app/src/components/collections/CollectionTagModal.test.js b/app/src/components/collections/CollectionTagModal.test.js index a44a961a..0a640152 100644 --- a/app/src/components/collections/CollectionTagModal.test.js +++ b/app/src/components/collections/CollectionTagModal.test.js @@ -439,4 +439,239 @@ describe('CollectionTagModal', () => { expect(mockOnClose).toHaveBeenCalled(); }); }); + + describe('hasDirectory prop', () => { + it('hides move toggle and directory input when hasDirectory is false', () => { + // TDD test - should FAIL initially because hasDirectory prop doesn't exist yet + render( + + ); + + // Toggle and directory input should be hidden + expect(screen.queryByTestId('toggle')).not.toBeInTheDocument(); + expect(screen.queryByRole('textbox')).not.toBeInTheDocument(); + }); + + it('shows move toggle and directory input when hasDirectory is true', () => { + render( + + ); + + // Toggle and directory input should be visible + expect(screen.getByTestId('toggle')).toBeInTheDocument(); + expect(screen.getByRole('textbox')).toBeInTheDocument(); + }); + + it('defaults hasDirectory to true for backward compatibility', () => { + // When hasDirectory is not specified, it should default to true (show directory UI) + render( + + ); + + // Toggle and directory input should be visible by default + expect(screen.getByTestId('toggle')).toBeInTheDocument(); + expect(screen.getByRole('textbox')).toBeInTheDocument(); + }); + + it('calls onSave with null directory when hasDirectory is false', async () => { + const mockOnClose = jest.fn(); + const mockOnSave = jest.fn().mockResolvedValue(undefined); + const mockGetTagInfo = jest.fn().mockResolvedValue({ + suggested_directory: null, + conflict: false, + }); + + render( + + ); + + // Add a tag + const addTagButton = screen.getByTestId('add-tag'); + await userEvent.click(addTagButton); + + // Wait for getTagInfo to be called + await waitFor(() => { + expect(mockGetTagInfo).toHaveBeenCalled(); + }); + + // Click save button + await act(async () => { + await userEvent.click(screen.getByTestId('api-button-Save')); + }); + + // Verify onSave was called with null directory + expect(mockOnSave).toHaveBeenCalledWith('test-tag', null); + expect(mockOnClose).toHaveBeenCalled(); + }); + }); + + describe('Tag Removal Save Behavior', () => { + it('calls onSave with updated directory after removing tag', async () => { + const mockOnSave = jest.fn().mockResolvedValue(undefined); + const mockOnClose = jest.fn(); + const mockGetTagInfo = jest.fn().mockResolvedValue({ + suggested_directory: '/videos/wrolpi.org', // Untagged directory + conflict: false, + conflict_message: null, + }); + + render( + + ); + + // Remove the tag - this triggers getTagInfo(null) and should update directory + await userEvent.click(screen.getByTestId('remove-tag')); + + // Wait for directory to be updated with the untagged directory + await waitFor(() => { + const input = screen.getByRole('textbox'); + expect(input).toHaveValue('/videos/wrolpi.org'); + }); + + // Click Move button to save + await act(async () => { + await userEvent.click(screen.getByTestId('api-button-Move')); + }); + + // Verify onSave was called with null tag and NEW directory + expect(mockOnSave).toHaveBeenCalledWith(null, '/videos/wrolpi.org'); + }); + + it('calls onSave with updated directory even when clicking save quickly after removing tag', async () => { + // This test simulates a potential race condition where user clicks save + // very quickly after removing the tag, before the async getTagInfo completes + const mockOnSave = jest.fn().mockResolvedValue(undefined); + const mockOnClose = jest.fn(); + + // Simulate a slow API call + const mockGetTagInfo = jest.fn().mockImplementation(() => { + return new Promise(resolve => { + setTimeout(() => { + resolve({ + suggested_directory: '/videos/wrolpi.org', + conflict: false, + conflict_message: null, + }); + }, 100); // 100ms delay + }); + }); + + render( + + ); + + // Remove the tag + await userEvent.click(screen.getByTestId('remove-tag')); + + // Wait for the directory to update (ensures async completed) + await waitFor(() => { + const input = screen.getByRole('textbox'); + expect(input).toHaveValue('/videos/wrolpi.org'); + }, { timeout: 500 }); + + // Click Move button + await act(async () => { + await userEvent.click(screen.getByTestId('api-button-Move')); + }); + + // Verify onSave was called with null tag and NEW directory + expect(mockOnSave).toHaveBeenCalledWith(null, '/videos/wrolpi.org'); + }); + + it('does NOT send stale directory if user clicks save before async completes', async () => { + // This test checks what happens if user manages to click save BEFORE getTagInfo resolves + // This would be the actual bug scenario + const mockOnSave = jest.fn().mockResolvedValue(undefined); + const mockOnClose = jest.fn(); + + // Create a promise that we can control + let resolveGetTagInfo; + const mockGetTagInfo = jest.fn().mockImplementation(() => { + return new Promise(resolve => { + resolveGetTagInfo = resolve; + }); + }); + + render( + + ); + + // Remove the tag - triggers getTagInfo but doesn't resolve yet + await userEvent.click(screen.getByTestId('remove-tag')); + + // getTagInfo should have been called + expect(mockGetTagInfo).toHaveBeenCalledWith(null); + + // At this point, directory is still the old value + const input = screen.getByRole('textbox'); + expect(input).toHaveValue('/videos/WROL/wrolpi.org'); + + // Now resolve the getTagInfo promise + await act(async () => { + resolveGetTagInfo({ + suggested_directory: '/videos/wrolpi.org', + conflict: false, + conflict_message: null, + }); + }); + + // Wait for directory to update + await waitFor(() => { + expect(input).toHaveValue('/videos/wrolpi.org'); + }); + + // NOW click save + await act(async () => { + await userEvent.click(screen.getByTestId('api-button-Move')); + }); + + // Verify onSave was called with the CORRECT directory + expect(mockOnSave).toHaveBeenCalledWith(null, '/videos/wrolpi.org'); + }); + }); }); diff --git a/modules/archive/lib.py b/modules/archive/lib.py index 7fe31195..497ca454 100644 --- a/modules/archive/lib.py +++ b/modules/archive/lib.py @@ -123,15 +123,8 @@ class DomainsConfig(ConfigFile): collection_data = collection_data.copy() collection_data['kind'] = 'domain' - # Warn if tag_name provided without directory - tag_name = collection_data.get('tag_name') - directory = collection_data.get('directory') - if tag_name and not directory: - logger.warning( - f"Domain collection '{name}' has tag_name '{tag_name}' " - f"but no directory - tags require a directory. Tag will be ignored." - ) - collection_data.pop('tag_name', None) + # Note: Collections can now be tagged even without a directory + # Tags enable UI search/filtering for directory-less collections valid_data_list.append(collection_data) except Exception as e: diff --git a/modules/archive/test/test_domain_tagging.py b/modules/archive/test/test_domain_tagging.py index 5d36b4b2..9ec7237b 100644 --- a/modules/archive/test/test_domain_tagging.py +++ b/modules/archive/test/test_domain_tagging.py @@ -20,22 +20,6 @@ async def test_create_domain_collection_with_directory(test_session, test_direct test_session.commit() assert collection.directory == domain_dir - assert collection.can_be_tagged is True - - -@pytest.mark.asyncio -async def test_create_unrestricted_domain_collection(test_session): - """Unrestricted domain collection (no directory) cannot be tagged.""" - collection = Collection( - name='example.com', - kind='domain', - directory=None # Unrestricted - ) - test_session.add(collection) - test_session.commit() - - assert collection.directory is None - assert collection.can_be_tagged is False @pytest.mark.asyncio @@ -63,8 +47,8 @@ async def test_tag_domain_collection_with_directory(async_client, test_session, @pytest.mark.asyncio -async def test_cannot_tag_unrestricted_domain_collection(async_client, test_session, tag_factory): - """Domain collection without directory cannot be tagged.""" +async def test_tag_unrestricted_domain_collection(async_client, test_session, tag_factory): + """Domain collection without directory can be tagged for UI searchability.""" collection = Collection( name='example.com', kind='domain', @@ -74,10 +58,14 @@ async def test_cannot_tag_unrestricted_domain_collection(async_client, test_sess tag = await tag_factory(name='News') test_session.commit() - assert collection.can_be_tagged is False + # Collections can now be tagged even without a directory + collection.set_tag('News') + test_session.commit() - with pytest.raises(ValueError, match='Cannot tag domain collection.*without a directory'): - collection.set_tag('News') + assert collection.tag is not None + assert collection.tag.name == 'News' + # Directory should still be None + assert collection.directory is None @pytest.mark.asyncio @@ -169,13 +157,11 @@ collections: assert 'example.com' in str(collection.directory) assert collection.tag is not None assert collection.tag.name == 'News' - assert collection.can_be_tagged is True @pytest.mark.asyncio -async def test_domain_config_warns_tag_without_directory(test_session, test_directory, tag_factory, caplog, - async_client): - """Config warns when tag_name provided without directory.""" +async def test_domain_config_with_tag_without_directory(test_session, test_directory, tag_factory, async_client): + """Config can have tag_name without directory - tag enables UI search.""" from modules.archive.lib import DomainsConfig # Create config file with tag but no directory @@ -195,18 +181,16 @@ collections: domains_config = DomainsConfig() domains_config.import_config(config_file) - # Verify collection created without tag + # Verify collection created with tag but no directory collection = test_session.query(Collection).filter_by( name='example.com', kind='domain' ).one() assert collection.directory is None - assert collection.tag is None # Tag ignored - assert collection.can_be_tagged is False - - # Check warning was logged - assert "tags require a directory" in caplog.text + # Tag is now applied even without directory - enables UI search + assert collection.tag is not None + assert collection.tag.name == 'News' @pytest.mark.asyncio @@ -268,7 +252,6 @@ async def test_get_or_create_domain_collection_with_directory(test_session, test assert collection.name == 'test.com' assert collection.kind == 'domain' assert collection.directory == domain_dir - assert collection.can_be_tagged is True @pytest.mark.asyncio @@ -311,3 +294,62 @@ async def test_get_archive_destination_unrestricted(test_session): expected = get_archive_directory() / 'example.com' assert destination == expected + + +@pytest.mark.asyncio +async def test_tag_domain_without_directory_saves_config(test_session, test_directory, tag_factory, await_switches): + """Tagging domain collection without directory should still save config.""" + from modules.archive.lib import domains_config + from wrolpi.collections.lib import tag_collection + import yaml + + # Create domain collection without directory + collection = Collection(name='example.com', kind='domain', directory=None) + test_session.add(collection) + await tag_factory(name='News') + test_session.commit() + + # Tag using library function (no directory) + tag_collection(collection.id, tag_name='News', directory=None, session=test_session) + test_session.commit() + + # Wait for switches to process + await await_switches() + + # Verify config was saved (file created by switch, just read it) + config_file = domains_config.get_file() + data = yaml.safe_load(config_file.read_text()) + + domain_entry = next((c for c in data.get('collections', []) if c['name'] == 'example.com'), None) + assert domain_entry is not None, "Domain collection not found in config" + assert domain_entry.get('tag_name') == 'News', "Tag name not saved in config" + + +@pytest.mark.asyncio +async def test_untag_domain_without_directory_saves_config(test_session, test_directory, tag_factory, await_switches): + """Untagging domain collection without directory should still save config.""" + from modules.archive.lib import domains_config + from wrolpi.collections.lib import tag_collection + import yaml + + # Create domain collection without directory, already tagged + collection = Collection(name='example.com', kind='domain', directory=None) + test_session.add(collection) + tag = await tag_factory(name='News') + collection.tag = tag + test_session.commit() + + # Untag using library function (tag_name=None) + tag_collection(collection.id, tag_name=None, directory=None, session=test_session) + test_session.commit() + + # Wait for switches to process + await await_switches() + + # Verify config was saved without tag (file created by switch, just read it) + config_file = domains_config.get_file() + data = yaml.safe_load(config_file.read_text()) + + domain_entry = next((c for c in data.get('collections', []) if c['name'] == 'example.com'), None) + assert domain_entry is not None, "Domain collection not found in config" + assert domain_entry.get('tag_name') is None, "Tag should be removed from config" diff --git a/modules/videos/channel/lib.py b/modules/videos/channel/lib.py index fad3abe8..347f1ac0 100644 --- a/modules/videos/channel/lib.py +++ b/modules/videos/channel/lib.py @@ -283,6 +283,8 @@ async def tag_channel(tag_name: str | None, directory: pathlib.Path | None, chan save_channels_config.activate_switch() else: logger.info(f'Tagging {channel} with {tag_name}') + # Always save config when tag changes, even without directory + save_channels_config.activate_switch() @optional_session diff --git a/modules/videos/channel/test/test_lib.py b/modules/videos/channel/test/test_lib.py index 80a7650f..c3c1ff4a 100644 --- a/modules/videos/channel/test/test_lib.py +++ b/modules/videos/channel/test/test_lib.py @@ -146,3 +146,56 @@ async def test_tag_channel_existing(async_client, test_session, test_directory, with test_channels_config.open() as fh: config = yaml.load(fh, Loader=yaml.Loader) assert config['channels'][0]['tag_name'] == tag.name + + +@pytest.mark.asyncio +async def test_tag_channel_without_directory_saves_config( + test_session, test_directory, channel_factory, tag_factory, test_channels_config, await_switches +): + """Tagging channel without providing directory should still save config.""" + channel_directory = test_directory / 'videos' / 'Channel Name' + channel = channel_factory(name='Channel Name', directory=channel_directory) + test_session.commit() + await await_switches() + + tag = await tag_factory(name='Tech') + + # Tag without providing directory (directory=None) + await lib.tag_channel(tag.name, None, channel.id, test_session) + test_session.commit() + await await_switches() + + # Channel's tag should be saved to the config even without directory + with test_channels_config.open() as fh: + config = yaml.load(fh, Loader=yaml.Loader) + channel_entry = next((c for c in config.get('channels', []) if c['name'] == 'Channel Name'), None) + assert channel_entry is not None, "Channel not found in config" + assert channel_entry.get('tag_name') == 'Tech', "Tag name not saved in config" + + +@pytest.mark.asyncio +async def test_untag_channel_without_directory_saves_config( + test_session, test_directory, channel_factory, tag_factory, test_channels_config, await_switches +): + """Untagging channel without providing directory should still save config.""" + channel_directory = test_directory / 'videos' / 'Channel Name' + channel = channel_factory(name='Channel Name', directory=channel_directory) + tag = await tag_factory(name='Tech') + channel.set_tag(tag.name) + test_session.commit() + await await_switches() + + # Verify tag is set + assert channel.tag is not None + + # Untag without providing directory (tag_name=None, directory=None) + await lib.tag_channel(None, None, channel.id, test_session) + test_session.commit() + await await_switches() + + # Channel's tag should be removed in the config + with test_channels_config.open() as fh: + config = yaml.load(fh, Loader=yaml.Loader) + channel_entry = next((c for c in config.get('channels', []) if c['name'] == 'Channel Name'), None) + assert channel_entry is not None, "Channel not found in config" + assert channel_entry.get('tag_name') is None, "Tag should be removed from config" diff --git a/wrolpi/collections/lib.py b/wrolpi/collections/lib.py index 526b7ae5..01276377 100644 --- a/wrolpi/collections/lib.py +++ b/wrolpi/collections/lib.py @@ -302,7 +302,7 @@ def refresh_collection(collection_id: int, send_events: bool = True) -> None: Events.send_directory_refresh(f'Refreshing: {relative_dir}') -@optional_session +@optional_session(commit=True) def tag_collection( collection_id: int, tag_name: Optional[str] = None, @@ -333,22 +333,35 @@ def tag_collection( # If tag_name is None, remove the tag from the collection if tag_name is None: collection.tag_id = None + + # Update directory if provided (user may want to move files when removing tag) + old_directory = collection.directory + if directory: + target_directory = validate_collection_directory(directory) + collection.directory = target_directory + else: + target_directory = collection.directory + session.flush() - # Trigger domain config save if this is a domain collection + # Trigger config save based on collection kind if collection.kind == 'domain': # Local import to avoid circular import: collections -> archive -> collections from modules.archive.lib import save_domains_config save_domains_config.activate_switch() + elif collection.kind == 'channel': + # Local import to avoid circular import: collections -> videos -> collections + from modules.videos.lib import save_channels_config + save_channels_config.activate_switch() # Return info about the un-tagging operation - relative_dir = get_relative_to_media_directory(collection.directory) if collection.directory else None + relative_dir = get_relative_to_media_directory(target_directory) if target_directory else None return { 'collection_id': collection.id, 'collection_name': collection.name, 'tag_name': None, 'directory': str(relative_dir) if relative_dir else None, - 'will_move_files': False, + 'will_move_files': target_directory is not None and old_directory != target_directory, } # Get or create the tag @@ -366,36 +379,36 @@ def tag_collection( # Use existing directory target_directory = collection.directory else: - # Suggest a directory based on collection type and name - if collection.kind == 'domain': - # Local import to avoid circular import: collections -> archive -> collections - from modules.archive.lib import get_archive_directory - base_dir = get_archive_directory() - else: - base_dir = get_media_directory() - - target_directory = base_dir / collection.name + # No directory provided and collection has none - keep it that way + # Collections can be tagged without a directory for UI search/filtering + target_directory = None # Apply the tag collection.tag_id = tag.id - collection.directory = target_directory + # Only update directory if we have a target (don't auto-generate for directory-less collections) + if target_directory is not None: + collection.directory = target_directory session.flush() - # Trigger domain config save if this is a domain collection + # Trigger config save based on collection kind if collection.kind == 'domain': # Local import to avoid circular import: collections -> archive -> collections from modules.archive.lib import save_domains_config save_domains_config.activate_switch() + elif collection.kind == 'channel': + # Local import to avoid circular import: collections -> videos -> collections + from modules.videos.lib import save_channels_config + save_channels_config.activate_switch() # Return info about the tagging operation - relative_dir = get_relative_to_media_directory(target_directory) + relative_dir = get_relative_to_media_directory(target_directory) if target_directory else None return { 'collection_id': collection.id, 'collection_name': collection.name, 'tag_name': tag_name, - 'directory': str(relative_dir), - 'will_move_files': collection.directory != target_directory, + 'directory': str(relative_dir) if relative_dir else None, + 'will_move_files': target_directory is not None and collection.directory != target_directory, } @@ -409,6 +422,7 @@ def get_tag_info( Get information about tagging a collection with a specific tag. Returns the suggested directory and checks for conflicts with existing collections. + For collections without a directory, returns suggested_directory=None. Args: collection_id: The collection ID @@ -426,6 +440,14 @@ def get_tag_info( if not collection: raise UnknownCollection(f"Collection with ID {collection_id} not found") + # For collections without a directory, return None - no directory suggestions needed + if collection.directory is None: + return { + 'suggested_directory': None, + 'conflict': False, + 'conflict_message': None, + } + # Use the collection's format_directory method to get the suggested directory suggested_directory = collection.format_directory(tag_name) diff --git a/wrolpi/collections/models.py b/wrolpi/collections/models.py index e8fd1cd6..03ae13d6 100644 --- a/wrolpi/collections/models.py +++ b/wrolpi/collections/models.py @@ -349,8 +349,9 @@ class Collection(ModelHelper, Base): ).all() existing_by_directory = {str(c.directory): c for c in existing} - # Step 3: Pre-fetch existing collections by (name, kind) for directory-less collections - name_kind_pairs = [(d['name'], d.get('kind', 'channel')) for d in data_list if not d.get('directory')] + # Step 3: Pre-fetch existing collections by (name, kind) for ALL collections + # This handles cases where directory changed or directory-less collections + name_kind_pairs = [(d['name'], d.get('kind', 'channel')) for d in data_list] existing_by_name_kind = {} if name_kind_pairs: from sqlalchemy import and_, or_ @@ -400,10 +401,12 @@ class Collection(ModelHelper, Base): directory_str = None # Find existing collection from pre-fetched data + # Priority: 1) by directory (if provided), 2) by (name, kind) as fallback collection = None if directory_str: collection = existing_by_directory.get(directory_str) - else: + # Always check by (name, kind) as fallback - handles directory changes + if not collection: collection = existing_by_name_kind.get((name, kind)) if collection: @@ -783,16 +786,9 @@ class Collection(ModelHelper, Base): def set_tag(self, tag_id_or_name: int | str | None) -> Optional[Tag]: """Assign or clear the Tag on this Collection. Mirrors Channel.set_tag semantics. - Raises: - ValueError: If attempting to tag a domain collection without a directory. + All collections can be tagged regardless of directory status. Tags enable + UI search/filtering even for directory-less collections. """ - # Validate domain collections can only be tagged if they have a directory - if self.kind == 'domain' and self.directory is None and tag_id_or_name is not None: - raise ValueError( - f"Cannot tag domain collection '{self.name}' without a directory. " - "Set collection.directory first or use an unrestricted domain collection." - ) - session = Session.object_session(self) if tag_id_or_name is None: self.tag = None @@ -803,20 +799,6 @@ class Collection(ModelHelper, Base): self.tag_id = self.tag.id if self.tag else None return self.tag - @property - def can_be_tagged(self) -> bool: - """Check if this Collection can be tagged. - - Domain collections require a directory to be tagged. - Other collection types can always be tagged. - - Returns: - True if this collection can be tagged, False otherwise. - """ - if self.kind == 'domain': - return self.directory is not None - return True - def __json__(self) -> dict: """Return JSON-serializable dict for API responses. @@ -838,7 +820,6 @@ class Collection(ModelHelper, Base): 'directory': directory_str, 'description': self.description, 'tag_name': self.tag.name if self.tag else None, - 'can_be_tagged': self.can_be_tagged, 'item_count': self.item_count, 'total_size': self.total_size, 'downloads': self.downloads, diff --git a/wrolpi/collections/test/test_api.py b/wrolpi/collections/test/test_api.py index 52b741dc..550ef083 100644 --- a/wrolpi/collections/test/test_api.py +++ b/wrolpi/collections/test/test_api.py @@ -427,3 +427,73 @@ class TestCollectionTagging: # Verify the tag has been removed assert collection.tag_id is None assert collection.tag_name is None + + @pytest.mark.asyncio + async def test_tag_collection_without_directory( + self, async_client, test_session, tag_factory + ): + """Test that tagging a collection without a directory works.""" + # Create a tag + tag = await tag_factory('TestTag') + test_session.flush() + + # Create a domain collection WITHOUT a directory + collection = Collection( + name='example.com', + kind='domain', + directory=None # No directory! + ) + test_session.add(collection) + test_session.commit() + + # Verify collection has no directory + assert collection.directory is None + + # Send POST request to tag the collection + request, response = await async_client.post( + f'/api/collections/{collection.id}/tag', + json={'tag_name': 'TestTag'} + ) + + # Check response - should succeed + assert response.status_code == HTTPStatus.OK + + # Refresh the collection from database + test_session.refresh(collection) + + # Verify the tag was set + assert collection.tag_id == tag.id + assert collection.tag_name == 'TestTag' + # Directory should still be None - not auto-generated + assert collection.directory is None + + @pytest.mark.asyncio + async def test_get_tag_info_without_directory( + self, async_client, test_session + ): + """Test that get_tag_info returns None for suggested_directory when collection has no directory.""" + # Create a domain collection WITHOUT a directory + collection = Collection( + name='example.com', + kind='domain', + directory=None # No directory! + ) + test_session.add(collection) + test_session.commit() + + # Request tag info + request, response = await async_client.post( + f'/api/collections/{collection.id}/tag_info', + json={'tag_name': 'TestTag'} + ) + + # Check response + assert response.status_code == HTTPStatus.OK + data = response.json + + # For collections without a directory, suggested_directory should be None + # (Currently it always computes a directory path) + assert data['suggested_directory'] is None + # No conflict checking needed without a directory + assert data['conflict'] is False + assert data['conflict_message'] is None diff --git a/wrolpi/collections/test/test_collection_tagging.py b/wrolpi/collections/test/test_collection_tagging.py index 6324b2bb..ae4339e5 100644 --- a/wrolpi/collections/test/test_collection_tagging.py +++ b/wrolpi/collections/test/test_collection_tagging.py @@ -143,3 +143,79 @@ async def test_tagging_unrestricted_collection_updates_config_only( entry = next(c for c in dumped if c.get('name') == 'Loose Stuff') assert entry.get('tag_name') == 'Favorites' assert 'directory' not in entry + + +def test_set_tag_on_directory_less_domain_collection(test_session): + """Test that set_tag works on domain collections without a directory.""" + from wrolpi.tags import Tag + + # Create a domain collection without a directory + collection = Collection(name='example.com', kind='domain', directory=None) + test_session.add(collection) + + # Create a tag directly + tag = Tag(name='TestTag', color='#ff0000') + test_session.add(tag) + test_session.commit() + + # This should NOT raise ValueError - it should successfully set the tag + # (Currently raises: ValueError: Cannot tag domain collection 'example.com' without a directory) + collection.set_tag('TestTag') + test_session.commit() + + # Verify tag was set + assert collection.tag_id == tag.id + assert collection.tag_name == 'TestTag' + # Directory should still be None + assert collection.directory is None + + +@pytest.mark.asyncio +async def test_tag_collection_removes_tag_and_updates_directory(test_session, test_directory, tag_factory, await_switches): + """When removing a tag via tag_collection, the directory should be updated if provided.""" + from wrolpi.collections.lib import tag_collection + + # Create a domain collection with a tag and tagged directory + tagged_dir = test_directory / 'archive' / 'News' / 'example.com' + tagged_dir.mkdir(parents=True, exist_ok=True) + + collection = Collection( + name='example.com', + kind='domain', + directory=tagged_dir, + ) + test_session.add(collection) + + # Create tag using factory + tag = await tag_factory(name='News') + collection.tag = tag + test_session.commit() + + # Verify initial state + assert collection.tag_name == 'News' + assert 'News' in str(collection.directory) + + # New directory without the tag + untagged_dir = test_directory / 'archive' / 'example.com' + untagged_dir.mkdir(parents=True, exist_ok=True) + + # Remove the tag AND update the directory + result = tag_collection( + collection_id=collection.id, + tag_name=None, # Remove tag + directory=str(untagged_dir), # New directory + session=test_session + ) + test_session.commit() + await await_switches() + + # Verify tag was removed + assert collection.tag_id is None + assert collection.tag_name is None + + # Verify directory was updated (THIS IS THE BUG - directory is not updated) + assert collection.directory == untagged_dir, \ + f"Expected directory to be {untagged_dir}, but got {collection.directory}" + + # Verify result contains correct directory + assert result['directory'] == str(untagged_dir.relative_to(test_directory)) diff --git a/wrolpi/common.py b/wrolpi/common.py index 94e196cf..3ab4cc01 100644 --- a/wrolpi/common.py +++ b/wrolpi/common.py @@ -546,7 +546,9 @@ class ConfigFile: raise RuntimeError(f'Refusing to save config because it was never successfully imported! {rel_path}') version = self.read_config_file(file).get('version') if version and version > self.version: - raise RuntimeError(f'Refusing to overwrite newer config ({rel_path}): {version} > {self.version}') + message = f'Refusing to overwrite newer config ({rel_path}): {version} > {self.version}' + Events.send_config_save_failed(message) + raise RuntimeError(message) # Don't overwrite a real config while testing. if PYTEST and not is_tempfile(file): diff --git a/wrolpi/contexts.py b/wrolpi/contexts.py index 5eb53a2e..b205738b 100644 --- a/wrolpi/contexts.py +++ b/wrolpi/contexts.py @@ -29,6 +29,7 @@ def attach_shared_contexts(app: Sanic): app.shared_ctx.channels_config = manager.dict() app.shared_ctx.download_manager_config = manager.dict() app.shared_ctx.videos_downloader_config = manager.dict() + app.shared_ctx.domains_config = manager.dict() # Shared dicts. app.shared_ctx.refresh = manager.dict() app.shared_ctx.uploaded_files = manager.dict() @@ -84,6 +85,7 @@ def reset_shared_contexts(app: Sanic): app.shared_ctx.channels_config.clear() app.shared_ctx.download_manager_config.clear() app.shared_ctx.videos_downloader_config.clear() + app.shared_ctx.domains_config.clear() # Shared dicts. app.shared_ctx.refresh.clear() app.shared_ctx.uploaded_files.clear() @@ -180,3 +182,9 @@ def initialize_configs_contexts(app: Sanic): DOWNLOAD_MANAGER_CONFIG.initialize(app.shared_ctx.download_manager_config) except Exception as e: logger.error(f'Failed to initialize in-memory download manager config: {e}') + + try: + from modules.archive.lib import domains_config + domains_config.initialize(app.shared_ctx.domains_config) + except Exception as e: + logger.error(f'Failed to initialize in-memory domains config: {e}') diff --git a/wrolpi/db.py b/wrolpi/db.py index 33d8dfea..5efbed74 100644 --- a/wrolpi/db.py +++ b/wrolpi/db.py @@ -157,10 +157,8 @@ def optional_session(commit: Union[callable, bool] = False) -> callable: if commit: session.commit() else: - with get_db_session() as session: + with get_db_session(commit=commit) as session: result = func(*args, session=session, **kwargs) - if commit: - session.commit() return result def wrapper(*w_args, **w_kwargs): diff --git a/wrolpi/files/test/test_ebooks.py b/wrolpi/files/test/test_ebooks.py index 1ac4cc02..76aeadc1 100644 --- a/wrolpi/files/test/test_ebooks.py +++ b/wrolpi/files/test/test_ebooks.py @@ -113,7 +113,8 @@ async def test_search_ebooks(test_session, async_client, example_epub): @pytest.mark.asyncio -async def test_discover_calibre_cover(test_session, test_directory, example_epub, example_mobi, image_file): +async def test_discover_calibre_cover(test_session, async_client, test_directory, example_epub, example_mobi, + image_file): """Calibre puts a cover near an ebook file, test if it can be found.""" # Create a Calibre metadata file. metadata = test_directory / 'metadata.opf' diff --git a/wrolpi/test/test_contexts.py b/wrolpi/test/test_contexts.py new file mode 100644 index 00000000..ee90c5ef --- /dev/null +++ b/wrolpi/test/test_contexts.py @@ -0,0 +1,42 @@ +"""Tests for shared context initialization.""" + +import pytest + + +def test_domains_config_in_shared_context(): + """Verify domains_config is initialized in shared context. + + This is critical for multi-process deployments. Without proper initialization, + domains_config.version won't be shared across processes, causing config save failures. + + The bug this catches: domains_config was missing from attach_shared_contexts() and + initialize_configs_contexts() in wrolpi/contexts.py, causing version tracking to fail + in production multi-process environment. + """ + from sanic import Sanic + from wrolpi.contexts import attach_shared_contexts, initialize_configs_contexts + + # Create a fresh Sanic app for this test to avoid polluting global state + test_app = Sanic(f"test_context_{id(test_domains_config_in_shared_context)}") + + # Initialize shared contexts as done in production + attach_shared_contexts(test_app) + initialize_configs_contexts(test_app) + + # Verify domains_config has a shared context + assert hasattr(test_app.shared_ctx, 'domains_config'), \ + "domains_config not found in shared_ctx - add it to attach_shared_contexts() in contexts.py" + + # Verify all other expected configs are also present + expected_configs = [ + 'wrolpi_config', + 'tags_config', + 'inventories_config', + 'channels_config', + 'download_manager_config', + 'videos_downloader_config', + 'domains_config', + ] + for config_name in expected_configs: + assert hasattr(test_app.shared_ctx, config_name), \ + f"{config_name} not found in shared_ctx"