diff --git a/wrolpi/collections/config.py b/wrolpi/collections/config.py index b91bce65..662cdd87 100644 --- a/wrolpi/collections/config.py +++ b/wrolpi/collections/config.py @@ -108,13 +108,20 @@ class PlaylistItemsSection(ConfigSection): key = self.item_key(name, item) except Exception as e: logger.error(f'Skipping invalid item of playlist {name!r}: {item}', exc_info=e) - continue - if key in rows: - logger.warning(f'Skipping duplicate item of playlist {name!r}: {item}') + key = None + if key is None or key in rows: + if key: + logger.warning(f'Skipping duplicate item of playlist {name!r}: {item}') + # Counted as skipped (the config is not saved over it); a kind of None is never an item's key. + invalid.add((name, None, position, None)) continue rows[key] = dict(position=position, title=item.get('title')) return rows, invalid + def present(self, config: dict, file_rows: dict, invalid_keys: set) -> bool: + # A listed playlist without items has none: only an empty or missing playlists list removes nothing. + return any(i.get('name') for i in config.get('playlists') or []) + @staticmethod def _query(session: Session, *columns): from modules.zim.models import Zim @@ -283,14 +290,14 @@ class PlaylistsConfig(ConfigFile): def _merge_items(self, diff: SectionDiff): """Without deletes, a playlist's items in the database keep their order, and the file's other items are - added after the item that precedes them in the file.""" + added after the item that precedes them in the file. Only titles are taken from the file's other items.""" section = PlaylistItemsSection() - if not diff.added: + if not (names := {i[0] for i in (*diff.added, *diff.changed)}): return with get_db_session() as session: db_rows = section.db_rows(session) file_rows, _ = section.file_rows(self._config) - for name in {i[0] for i in diff.added}: + for name in names: merged = sorted((i for i in db_rows if i[0] == name), key=lambda i: db_rows[i]['position']) previous = None for key in sorted((i for i in file_rows if i[0] == name), key=lambda i: file_rows[i]['position']): @@ -302,9 +309,12 @@ class PlaylistsConfig(ConfigFile): for position, key in enumerate(merged, start=1): if key in diff.added: diff.added[key]['position'] = position - elif db_rows[key]['position'] != position or key in diff.changed: - title = (diff.changed.get(key) or db_rows[key])['title'] - diff.changed[key] = dict(position=position, title=title) + continue + row = dict(position=position, title=(diff.changed.get(key) or db_rows[key])['title']) + if row == db_rows[key]: + diff.changed.pop(key, None) + else: + diff.changed[key] = row def import_config(self, file: pathlib.Path = None, send_events=False): self._config_outdated = False diff --git a/wrolpi/collections/test/test_playlists_config.py b/wrolpi/collections/test/test_playlists_config.py index 474c0507..f7f55ef3 100644 --- a/wrolpi/collections/test/test_playlists_config.py +++ b/wrolpi/collections/test/test_playlists_config.py @@ -392,3 +392,25 @@ async def test_import_without_deletes_keeps_database_items(test_session, test_di test_session.expire_all() collection = test_session.query(Collection).filter_by(name='Mix', kind='playlist').one() assert [i.url for i in collection.items] == ['/b', '/a', '/c'] + + +@pytest.mark.asyncio +async def test_import_without_deletes_keeps_database_order(test_session, test_directory, playlists_config): + """Without a database version, a file that only reorders a playlist's items (and lacks one the database has) + leaves the database's order as it is.""" + collection = Collection(name='Mix', kind='playlist') + test_session.add(collection) + test_session.flush([collection]) + collection.add_url(test_session, '/a', title='A') + collection.add_url(test_session, '/x', title='X') # Added after the file was saved. + collection.add_url(test_session, '/b', title='B') + test_session.commit() + playlists_config.get_file().write_text(yaml.dump(dict(version=1, playlists=[ + dict(name='Mix', items=[dict(url='/b', title='B'), dict(url='/a', title='A')]), + ]))) + + playlists_config.import_config() + + test_session.expire_all() + collection = test_session.query(Collection).filter_by(name='Mix', kind='playlist').one() + assert [(i.url, i.position) for i in collection.items] == [('/a', 1), ('/x', 2), ('/b', 3)] diff --git a/wrolpi/collections/test/test_playlists_config_import.py b/wrolpi/collections/test/test_playlists_config_import.py index 39d12f42..1c391858 100644 --- a/wrolpi/collections/test/test_playlists_config_import.py +++ b/wrolpi/collections/test/test_playlists_config_import.py @@ -215,3 +215,36 @@ async def test_refresh_changing_primary_file_counts(test_session, test_directory test_session.expire_all() assert test_session.query(FileGroup).one().primary_path != test_directory / 'video.mp4' assert get_config_version(test_session, 'playlists.yaml') > 0 + + +@pytest.mark.asyncio +async def test_empty_items_are_removed(test_session, saved_playlists): + """A listed playlist with no items has its items removed, even when no playlist in the file has an item (only + an empty or missing playlists list deletes nothing).""" + def change(data): + for playlist in data['playlists']: + playlist['items'] = [] + + edit_config(saved_playlists, change) + config = get_playlists_config() + config.import_config() + + assert config.successful_import is True + assert items(test_session, 'One') == [] + assert items(test_session, 'Two') == [] + + +@pytest.mark.asyncio +async def test_invalid_items_are_skipped(test_session, saved_playlists): + """An item that cannot be read (a Zim item without an entry), or that repeats another, is counted as skipped, + so the config is not saved over it.""" + def change(playlist): + playlist['items'].append(dict(zim='other.zim')) + playlist['items'].append(dict(url='/one')) + + edit_playlist(saved_playlists, 'One', change) + config = get_playlists_config() + config.import_config() + + assert config.import_skipped == 2 + assert items(test_session) == ONE diff --git a/wrolpi/common.py b/wrolpi/common.py index 9ade4921..0a09f368 100644 --- a/wrolpi/common.py +++ b/wrolpi/common.py @@ -844,7 +844,8 @@ class ConfigFile: file_rows = {i.name: i.file_rows(self._config) for i in self.sections} with get_db_session() as session: db_rows = {i.name: i.db_rows(session) for i in self.sections} - diffs = {i.name: diff_section(*file_rows[i.name], db_rows[i.name], deletes=decision == IMPORT, same=i.same) + diffs = {i.name: diff_section(*file_rows[i.name], db_rows[i.name], deletes=decision == IMPORT, same=i.same, + present=i.present(self._config, *file_rows[i.name])) for i in self.sections} # `prepare` may skip rows it cannot apply. self.import_skipped = sum(len(invalid) for _, invalid in file_rows.values()) diff --git a/wrolpi/config_import.py b/wrolpi/config_import.py index 138869d7..fb29ab09 100644 --- a/wrolpi/config_import.py +++ b/wrolpi/config_import.py @@ -49,6 +49,11 @@ class ConfigSection: deleted since the snapshot.""" raise NotImplementedError() + def present(self, config: dict, file_rows: dict, invalid_keys: set) -> bool: + """Whether the file lists this section at all; an empty section cannot be told apart from a lost or blanked + file, so it removes nothing.""" + return bool(file_rows or invalid_keys) + def same(self, file_row: dict, db_row: dict) -> bool: """Whether a file row is already in the database.""" return file_row == db_row @@ -60,16 +65,20 @@ class ConfigSection: def diff_section(file_rows: Dict[Hashable, dict], invalid_keys: Set[Hashable], db_rows: Dict[Hashable, dict], - deletes: bool, same: Callable[[dict, dict], bool] = operator.eq) -> SectionDiff: + deletes: bool, same: Callable[[dict, dict], bool] = operator.eq, present: bool = None) \ + -> SectionDiff: """Compare a section's file rows with its database rows. `deletes`: rows the file no longer lists are removed. - An empty section never removes anything: it cannot be told apart from a lost or blanked file.""" + A section that is not `present` (by default, an empty one) never removes anything: it cannot be told apart from + a lost or blanked file.""" diff = SectionDiff() for key, row in file_rows.items(): if key not in db_rows: diff.added[key] = row elif not same(row, db_rows[key]): diff.changed[key] = row - if deletes and (file_rows or invalid_keys): + if present is None: + present = bool(file_rows or invalid_keys) + if deletes and present: diff.removed = db_rows.keys() - file_rows.keys() - invalid_keys return diff