Fix #780: Deezer/non-Spotify organize-by-playlist resolved the wrong row
resolve_mirrored_playlist tried the mirrored-playlists primary key FIRST for any all-digit ref. Deezer upstream ids are all-numeric, so a Deezer playlist id was mistaken for the PK and the organize-by-playlist toggle resolved a wrong row (or nothing) — the toggle silently wouldn't save / 'Open in Mirrored' missed. Resolve by (source, source_playlist_id) first, fall back to PK only when the source lookup misses. Thread the batch/wishlist source through the download-path callers so numeric upstream ids resolve correctly there too. Spotify (base62 ids) is unaffected. Seam tests: numeric Deezer id resolves by source (not PK), spotify alphanumeric by source, PK fallback still works, profile-scoped, empty refs -> None.
This commit is contained in:
parent
0353d365d6
commit
a977d28144
4 changed files with 81 additions and 3 deletions
|
|
@ -373,6 +373,7 @@ def run_full_missing_tracks_process(batch_id, playlist_id, tracks_json, deps: Ma
|
||||||
playlist_name=batch_playlist_name,
|
playlist_name=batch_playlist_name,
|
||||||
batch_playlist_folder_mode=batch_playlist_folder_mode,
|
batch_playlist_folder_mode=batch_playlist_folder_mode,
|
||||||
profile_id=batch_profile_id,
|
profile_id=batch_profile_id,
|
||||||
|
source=batch_source,
|
||||||
)
|
)
|
||||||
if effective_playlist_folder_mode and not batch_playlist_folder_mode:
|
if effective_playlist_folder_mode and not batch_playlist_folder_mode:
|
||||||
with tasks_lock:
|
with tasks_lock:
|
||||||
|
|
@ -1043,10 +1044,12 @@ def run_full_missing_tracks_process(batch_id, playlist_id, tracks_json, deps: Ma
|
||||||
wl_source = {}
|
wl_source = {}
|
||||||
wl_pl_ref = wl_source.get('playlist_id')
|
wl_pl_ref = wl_source.get('playlist_id')
|
||||||
wl_pl_name = wl_source.get('playlist_name')
|
wl_pl_name = wl_source.get('playlist_name')
|
||||||
|
wl_pl_source = wl_source.get('source') or 'spotify'
|
||||||
if wl_pl_ref and hasattr(db, 'resolve_mirrored_playlist'):
|
if wl_pl_ref and hasattr(db, 'resolve_mirrored_playlist'):
|
||||||
wl_mirrored = db.resolve_mirrored_playlist(
|
wl_mirrored = db.resolve_mirrored_playlist(
|
||||||
wl_pl_ref,
|
wl_pl_ref,
|
||||||
profile_id=batch_profile_id,
|
profile_id=batch_profile_id,
|
||||||
|
default_source=wl_pl_source,
|
||||||
)
|
)
|
||||||
if wl_mirrored and wl_mirrored.get('organize_by_playlist'):
|
if wl_mirrored and wl_mirrored.get('organize_by_playlist'):
|
||||||
task_pl_folder_mode = True
|
task_pl_folder_mode = True
|
||||||
|
|
|
||||||
|
|
@ -101,6 +101,7 @@ def resolve_playlist_folder_mode_for_batch(
|
||||||
playlist_name: str,
|
playlist_name: str,
|
||||||
batch_playlist_folder_mode: bool,
|
batch_playlist_folder_mode: bool,
|
||||||
profile_id: int = 1,
|
profile_id: int = 1,
|
||||||
|
source: str = 'spotify',
|
||||||
) -> tuple[bool, str]:
|
) -> tuple[bool, str]:
|
||||||
"""Merge batch flag with persisted mirrored-playlist preference."""
|
"""Merge batch flag with persisted mirrored-playlist preference."""
|
||||||
if batch_playlist_folder_mode:
|
if batch_playlist_folder_mode:
|
||||||
|
|
@ -109,7 +110,11 @@ def resolve_playlist_folder_mode_for_batch(
|
||||||
if not hasattr(db, 'resolve_mirrored_playlist'):
|
if not hasattr(db, 'resolve_mirrored_playlist'):
|
||||||
return False, playlist_name
|
return False, playlist_name
|
||||||
|
|
||||||
mirrored = db.resolve_mirrored_playlist(playlist_id, profile_id=profile_id)
|
# Pass the batch's source so numeric upstream ids (e.g. Deezer) resolve by
|
||||||
|
# source instead of colliding with the mirrored-playlists primary key.
|
||||||
|
mirrored = db.resolve_mirrored_playlist(
|
||||||
|
playlist_id, profile_id=profile_id, default_source=source or 'spotify'
|
||||||
|
)
|
||||||
if mirrored and mirrored.get('organize_by_playlist'):
|
if mirrored and mirrored.get('organize_by_playlist'):
|
||||||
return True, mirrored.get('name') or playlist_name
|
return True, mirrored.get('name') or playlist_name
|
||||||
return False, playlist_name
|
return False, playlist_name
|
||||||
|
|
|
||||||
|
|
@ -12502,13 +12502,26 @@ class MusicDatabase:
|
||||||
*,
|
*,
|
||||||
default_source: str = 'spotify',
|
default_source: str = 'spotify',
|
||||||
) -> Optional[Dict]:
|
) -> Optional[Dict]:
|
||||||
"""Resolve a mirrored playlist from numeric id or upstream source id."""
|
"""Resolve a mirrored playlist from an upstream source id or numeric PK.
|
||||||
|
|
||||||
|
Resolves by ``(source, source_playlist_id)`` FIRST, then falls back to
|
||||||
|
treating an all-digit ref as the mirrored-playlists primary key. The
|
||||||
|
order matters: some sources (e.g. Deezer) use all-numeric upstream ids,
|
||||||
|
and the old PK-first logic mistook those for the PK — so the Deezer
|
||||||
|
organize-by-playlist toggle resolved the wrong row (or nothing).
|
||||||
|
"""
|
||||||
if playlist_ref is None or playlist_ref == '':
|
if playlist_ref is None or playlist_ref == '':
|
||||||
return None
|
return None
|
||||||
ref = str(playlist_ref).strip()
|
ref = str(playlist_ref).strip()
|
||||||
|
if not ref:
|
||||||
|
return None
|
||||||
|
if default_source:
|
||||||
|
row = self.get_mirrored_playlist_by_source(default_source, ref, profile_id)
|
||||||
|
if row:
|
||||||
|
return row
|
||||||
if ref.isdigit():
|
if ref.isdigit():
|
||||||
return self.get_mirrored_playlist(int(ref))
|
return self.get_mirrored_playlist(int(ref))
|
||||||
return self.get_mirrored_playlist_by_source(default_source, ref, profile_id)
|
return None
|
||||||
|
|
||||||
def set_mirrored_playlist_organize_by_playlist(
|
def set_mirrored_playlist_organize_by_playlist(
|
||||||
self,
|
self,
|
||||||
|
|
|
||||||
57
tests/test_resolve_mirrored_playlist.py
Normal file
57
tests/test_resolve_mirrored_playlist.py
Normal file
|
|
@ -0,0 +1,57 @@
|
||||||
|
"""Seam tests for resolve_mirrored_playlist source-vs-PK resolution.
|
||||||
|
|
||||||
|
Regression cover for the PR #780 follow-up: numeric *upstream* ids (Deezer
|
||||||
|
playlist ids are all-digit) must resolve by (source, source_playlist_id), NOT
|
||||||
|
be mistaken for the mirrored-playlists primary key. The old PK-first logic made
|
||||||
|
the Deezer organize-by-playlist toggle resolve the wrong row (or nothing).
|
||||||
|
"""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
from database.music_database import MusicDatabase
|
||||||
|
|
||||||
|
|
||||||
|
def test_numeric_source_id_resolves_by_source_not_pk(tmp_path):
|
||||||
|
"""A Deezer-style all-numeric upstream id resolves the right row."""
|
||||||
|
db = MusicDatabase(str(tmp_path / "m.db"))
|
||||||
|
pk = db.mirror_playlist(source='deezer', source_playlist_id='908622995',
|
||||||
|
name='My Deezer Mix', tracks=[], profile_id=1)
|
||||||
|
assert pk
|
||||||
|
row = db.resolve_mirrored_playlist('908622995', profile_id=1, default_source='deezer')
|
||||||
|
assert row is not None
|
||||||
|
assert row['id'] == pk
|
||||||
|
assert row['source'] == 'deezer'
|
||||||
|
# And it must NOT have been a PK lookup: 908622995 is not a valid PK here.
|
||||||
|
assert db.get_mirrored_playlist(908622995) is None
|
||||||
|
|
||||||
|
|
||||||
|
def test_spotify_alphanumeric_resolves_by_source(tmp_path):
|
||||||
|
db = MusicDatabase(str(tmp_path / "m.db"))
|
||||||
|
pk = db.mirror_playlist(source='spotify', source_playlist_id='37i9dQZF1DXcBWIGoYBM5M',
|
||||||
|
name='Top Hits', tracks=[], profile_id=1)
|
||||||
|
row = db.resolve_mirrored_playlist('37i9dQZF1DXcBWIGoYBM5M', profile_id=1, default_source='spotify')
|
||||||
|
assert row is not None and row['id'] == pk
|
||||||
|
|
||||||
|
|
||||||
|
def test_pk_fallback_when_no_source_match(tmp_path):
|
||||||
|
"""A numeric ref that isn't a known source id still resolves via PK fallback."""
|
||||||
|
db = MusicDatabase(str(tmp_path / "m.db"))
|
||||||
|
pk = db.mirror_playlist(source='spotify', source_playlist_id='abc123XYZ',
|
||||||
|
name='Sp', tracks=[], profile_id=1)
|
||||||
|
row = db.resolve_mirrored_playlist(str(pk), profile_id=1, default_source='spotify')
|
||||||
|
assert row is not None and row['id'] == pk
|
||||||
|
|
||||||
|
|
||||||
|
def test_resolution_is_profile_scoped(tmp_path):
|
||||||
|
db = MusicDatabase(str(tmp_path / "m.db"))
|
||||||
|
db.mirror_playlist(source='deezer', source_playlist_id='555',
|
||||||
|
name='D', tracks=[], profile_id=1)
|
||||||
|
# Another profile must not resolve profile 1's Deezer playlist by source.
|
||||||
|
assert db.resolve_mirrored_playlist('555', profile_id=2, default_source='deezer') is None
|
||||||
|
|
||||||
|
|
||||||
|
def test_empty_refs_return_none(tmp_path):
|
||||||
|
db = MusicDatabase(str(tmp_path / "m.db"))
|
||||||
|
assert db.resolve_mirrored_playlist(None) is None
|
||||||
|
assert db.resolve_mirrored_playlist('') is None
|
||||||
|
assert db.resolve_mirrored_playlist(' ') is None
|
||||||
Loading…
Reference in a new issue