Playlist items: keep database order without deletes; empty lists delete

Without a database version, every playlist with an added or changed item
keeps the database's order (the file's new items go after their
predecessor; only titles come from the file), and a row whose merged
position and title equal the database's is not written.  Before, only
playlists that gained an item were merged, so a file that only reordered
(or lacked an item the database had) gave two items one position.

A section can say it is present without rows (`ConfigSection.present`):
a listed playlist with an empty items list has its items removed; an
empty or missing playlists list still removes nothing.  An item that
cannot be read, or repeats another, is counted in import_skipped, so the
config is not saved over it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

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

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)]

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

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())

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