Clarify Spotify auth actions
- Hide the auth button when a Spotify session is active - Treat disconnect as a session change, not a provider swap - Share metadata source labels in the registry - Tighten rate-limit copy around Spotify-specific behavior
This commit is contained in:
Antti Kettunen
parent
7f191be7af
commit
9646f6ca7f
8 changed files with 88 additions and 50 deletions
|
|
@ -18,6 +18,13 @@ logger = get_logger("metadata.registry")
|
|||
MetadataClientFactory = Callable[[], Any]
|
||||
|
||||
METADATA_SOURCE_PRIORITY = ("deezer", "itunes", "spotify", "discogs", "hydrabase")
|
||||
METADATA_SOURCE_LABELS = {
|
||||
"spotify": "Spotify",
|
||||
"itunes": "iTunes",
|
||||
"deezer": "Deezer",
|
||||
"discogs": "Discogs",
|
||||
"hydrabase": "Hydrabase",
|
||||
}
|
||||
|
||||
_UNSET = object()
|
||||
_client_cache_lock = threading.RLock()
|
||||
|
|
@ -293,6 +300,17 @@ def get_primary_source(spotify_client_factory: Optional[MetadataClientFactory] =
|
|||
return source
|
||||
|
||||
|
||||
def get_spotify_disconnect_source() -> str:
|
||||
"""Return the active metadata source after Spotify is disconnected."""
|
||||
source = get_primary_source()
|
||||
return "deezer" if source == "spotify" else source
|
||||
|
||||
|
||||
def get_metadata_source_label(source: str) -> str:
|
||||
"""Return a human-readable label for a metadata source."""
|
||||
return METADATA_SOURCE_LABELS.get(source, source.replace("_", " ").title())
|
||||
|
||||
|
||||
def get_source_priority(preferred_source: str):
|
||||
"""Return source priority with preferred source first."""
|
||||
ordered = []
|
||||
|
|
|
|||
|
|
@ -17,7 +17,7 @@ from flask_socketio import SocketIO, join_room, leave_room
|
|||
# ---------------------------------------------------------------------------
|
||||
|
||||
_DEFAULT_STATUS_CACHE = {
|
||||
'spotify': {'connected': True, 'response_time': 12.5, 'source': 'spotify'},
|
||||
'spotify': {'connected': True, 'authenticated': True, 'response_time': 12.5, 'source': 'spotify'},
|
||||
'media_server': {'connected': True, 'response_time': 8.1, 'type': 'plex'},
|
||||
'soulseek': {'connected': True, 'response_time': 5.3, 'source': 'soulseek'},
|
||||
}
|
||||
|
|
|
|||
30
tests/metadata/test_metadata_registry.py
Normal file
30
tests/metadata/test_metadata_registry.py
Normal file
|
|
@ -0,0 +1,30 @@
|
|||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
sys.path.insert(0, str(Path(__file__).resolve().parents[2]))
|
||||
|
||||
from core.metadata import registry
|
||||
|
||||
|
||||
def test_spotify_disconnect_source_uses_deezer_when_spotify_is_primary(monkeypatch):
|
||||
monkeypatch.setattr(registry, "get_primary_source", lambda: "spotify")
|
||||
|
||||
assert registry.get_spotify_disconnect_source() == "deezer"
|
||||
|
||||
|
||||
def test_spotify_disconnect_source_keeps_non_spotify_primary(monkeypatch):
|
||||
monkeypatch.setattr(registry, "get_primary_source", lambda: "discogs")
|
||||
|
||||
assert registry.get_spotify_disconnect_source() == "discogs"
|
||||
|
||||
|
||||
def test_metadata_source_label_maps_known_sources():
|
||||
assert registry.get_metadata_source_label("spotify") == "Spotify"
|
||||
assert registry.get_metadata_source_label("itunes") == "iTunes"
|
||||
assert registry.get_metadata_source_label("deezer") == "Deezer"
|
||||
assert registry.get_metadata_source_label("discogs") == "Discogs"
|
||||
assert registry.get_metadata_source_label("hydrabase") == "Hydrabase"
|
||||
|
||||
|
||||
def test_metadata_source_label_falls_back_to_title_case():
|
||||
assert registry.get_metadata_source_label("apple_music") == "Apple Music"
|
||||
|
|
@ -56,6 +56,7 @@ class TestServiceStatus:
|
|||
assert 'media_server' in data
|
||||
assert 'soulseek' in data
|
||||
assert 'active_media_server' in data
|
||||
assert 'authenticated' in data['spotify']
|
||||
|
||||
def test_status_matches_http(self, test_app, shared_state):
|
||||
"""Socket event data matches HTTP endpoint response exactly."""
|
||||
|
|
|
|||
|
|
@ -98,7 +98,9 @@ from core.metadata.cache import get_metadata_cache
|
|||
from core.metadata import registry as metadata_registry
|
||||
from core.metadata.registry import (
|
||||
clear_cached_metadata_client,
|
||||
get_metadata_source_label,
|
||||
get_spotify_client,
|
||||
get_spotify_disconnect_source,
|
||||
register_runtime_clients,
|
||||
)
|
||||
from core.imports.context import (
|
||||
|
|
@ -800,11 +802,11 @@ _idle_since = {}
|
|||
_IDLE_GRACE_SECONDS = 5
|
||||
|
||||
_status_cache = {
|
||||
'spotify': {'connected': False, 'response_time': 0, 'source': 'itunes'},
|
||||
'spotify': {'connected': False, 'authenticated': False, 'response_time': 0, 'source': 'itunes'},
|
||||
'media_server': {'connected': False, 'response_time': 0, 'type': None},
|
||||
'soulseek': {'connected': False, 'response_time': 0},
|
||||
}
|
||||
_status_cache_timestamps = {
|
||||
_status_cache_timestamps: dict[str, float] = {
|
||||
'spotify': 0,
|
||||
'media_server': 0,
|
||||
'soulseek': 0,
|
||||
|
|
@ -3424,6 +3426,7 @@ def get_status():
|
|||
is_rate_limited = spotify_client.is_rate_limited() if spotify_client else False
|
||||
rate_limit_info = spotify_client.get_rate_limit_info() if (spotify_client and is_rate_limited) else None
|
||||
cooldown_remaining = spotify_client.get_post_ban_cooldown_remaining() if spotify_client else 0
|
||||
spotify_session_active = bool(spotify_client and getattr(spotify_client, 'sp', None) is not None)
|
||||
|
||||
# Read configured source once — no auth validation here, we do that explicitly below
|
||||
configured_source = config_manager.get('metadata.fallback_source', 'deezer') or 'deezer'
|
||||
|
|
@ -3445,6 +3448,7 @@ def get_status():
|
|||
|
||||
_status_cache['spotify'] = {
|
||||
'connected': True, # Always true — iTunes fallback is always available
|
||||
'authenticated': spotify_session_active,
|
||||
'response_time': round(spotify_response_time, 1),
|
||||
'source': music_source,
|
||||
'rate_limited': is_rate_limited,
|
||||
|
|
@ -4773,7 +4777,9 @@ def test_connection_endpoint():
|
|||
if success:
|
||||
current_time = time.time()
|
||||
if service == 'spotify':
|
||||
spotify_session_active = bool(spotify_client and getattr(spotify_client, 'sp', None) is not None)
|
||||
_status_cache['spotify']['connected'] = True
|
||||
_status_cache['spotify']['authenticated'] = spotify_session_active
|
||||
_status_cache['spotify']['source'] = _get_metadata_fallback_source()
|
||||
_status_cache_timestamps['spotify'] = current_time
|
||||
logger.info("Updated Spotify status cache after successful test")
|
||||
|
|
@ -4939,7 +4945,9 @@ def test_dashboard_connection_endpoint():
|
|||
if success:
|
||||
current_time = time.time()
|
||||
if service == 'spotify':
|
||||
spotify_session_active = bool(spotify_client and getattr(spotify_client, 'sp', None) is not None)
|
||||
_status_cache['spotify']['connected'] = True
|
||||
_status_cache['spotify']['authenticated'] = spotify_session_active
|
||||
_status_cache['spotify']['source'] = _get_metadata_fallback_source()
|
||||
_status_cache_timestamps['spotify'] = current_time
|
||||
logger.info("Updated Spotify status cache after successful dashboard test")
|
||||
|
|
@ -5838,7 +5846,7 @@ def spotify_callback():
|
|||
|
||||
@app.route('/api/spotify/disconnect', methods=['POST'])
|
||||
def spotify_disconnect():
|
||||
"""Disconnect Spotify and fall back to iTunes/Apple Music"""
|
||||
"""Disconnect Spotify and keep using the active primary metadata source."""
|
||||
global spotify_client
|
||||
try:
|
||||
# Pause enrichment worker before disconnecting to prevent it from hammering API
|
||||
|
|
@ -5846,18 +5854,20 @@ def spotify_disconnect():
|
|||
spotify_enrichment_worker.pause()
|
||||
spotify_client.disconnect()
|
||||
# Immediately update status cache so UI reflects the change
|
||||
fallback_src = _get_metadata_fallback_source()
|
||||
active_source = get_spotify_disconnect_source()
|
||||
source_label = get_metadata_source_label(active_source)
|
||||
_status_cache['spotify'] = {
|
||||
'connected': True, # Fallback source is always available
|
||||
'connected': False,
|
||||
'authenticated': False,
|
||||
'response_time': 0,
|
||||
'source': fallback_src,
|
||||
'source': active_source,
|
||||
'rate_limited': False,
|
||||
'rate_limit': None
|
||||
'rate_limit': None,
|
||||
'post_ban_cooldown': None
|
||||
}
|
||||
_status_cache_timestamps['spotify'] = time.time()
|
||||
fallback_label = 'Deezer' if fallback_src == 'deezer' else 'Discogs' if fallback_src == 'discogs' else 'iTunes'
|
||||
add_activity_item("", "Spotify Disconnected", f"Switched to {fallback_label} metadata source", "Now")
|
||||
return jsonify({'success': True, 'message': f'Spotify disconnected. Now using {fallback_label}.'})
|
||||
add_activity_item("", "Spotify Disconnected", f"Using {source_label} for metadata", "Now")
|
||||
return jsonify({'success': True, 'message': f'Spotify disconnected. Using {source_label} for metadata.', 'source': active_source, 'authenticated': False})
|
||||
except Exception as e:
|
||||
logger.error(f"Error disconnecting Spotify: {e}")
|
||||
return jsonify({'success': False, 'error': str(e)}), 500
|
||||
|
|
|
|||
|
|
@ -3693,7 +3693,7 @@
|
|||
<div class="api-service-frame">
|
||||
<h4 class="service-title" style="color: #e8e8e8;">Metadata Source</h4>
|
||||
<div class="form-group">
|
||||
<label>Primary Source:</label>
|
||||
<label>Primary metadata source:</label>
|
||||
<select id="metadata-fallback-source">
|
||||
<option value="spotify">Spotify</option>
|
||||
<option value="itunes">iTunes / Apple Music</option>
|
||||
|
|
@ -3702,7 +3702,7 @@
|
|||
</select>
|
||||
</div>
|
||||
<div class="callback-info">
|
||||
<div class="callback-help">The primary source for artist, album, and track metadata. Spotify requires authentication below. Discogs requires a personal token.</div>
|
||||
<div class="callback-help">Choose the primary source for artist, album, and track metadata. Spotify auth is optional and only needed for Spotify-specific actions. Discogs requires a personal token.</div>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
|
|
@ -3741,10 +3741,6 @@
|
|||
<button class="auth-button disconnect-button" id="spotify-disconnect-btn"
|
||||
onclick="disconnectSpotify()" style="display: none;">🔌
|
||||
Disconnect</button>
|
||||
<button class="auth-button disconnect-button"
|
||||
onclick="clearSpotifyCacheAndFallback()"
|
||||
title="Clear Spotify token cache and switch to your configured fallback metadata source">🗑️
|
||||
Clear Cache & Use Fallback</button>
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
|
|
@ -7864,13 +7860,13 @@
|
|||
<span class="rate-limit-value rate-limit-countdown" id="rate-limit-countdown">—</span>
|
||||
</div>
|
||||
</div>
|
||||
<p class="rate-limit-hint">You can wait for the ban to expire (the app uses Apple Music in the meantime) or disconnect Spotify to clear the ban immediately.</p>
|
||||
<p class="rate-limit-hint">While rate limiting is active, Spotify-specific features are unavailable. You can wait for the ban to expire or disconnect Spotify to clear it immediately.</p>
|
||||
</div>
|
||||
<div class="confirm-modal-actions rate-limit-modal-actions">
|
||||
<button class="modal-button modal-button--secondary" onclick="closeRateLimitModal()">Dismiss</button>
|
||||
<button class="modal-button rate-limit-disconnect-btn" onclick="disconnectSpotifyFromRateLimit()">
|
||||
Disconnect Spotify
|
||||
<span class="rate-limit-disconnect-sub">Clear ban, pause enrichment & switch to fallback source</span>
|
||||
Disconnect
|
||||
<span class="rate-limit-disconnect-sub">Clear the ban and pause Spotify-specific features</span>
|
||||
</button>
|
||||
</div>
|
||||
</div>
|
||||
|
|
|
|||
|
|
@ -3066,8 +3066,10 @@ async function authenticateSpotify() {
|
|||
}
|
||||
|
||||
async function disconnectSpotify() {
|
||||
const fallbackName = currentMusicSourceName !== 'Spotify' ? currentMusicSourceName : 'the configured fallback source';
|
||||
if (!await showConfirmDialog({ title: 'Disconnect Spotify', message: `Disconnect Spotify? The app will switch to ${fallbackName} for metadata.` })) {
|
||||
if (!await showConfirmDialog({
|
||||
title: 'Disconnect Spotify',
|
||||
message: 'Disconnect Spotify? Spotify-specific actions will stop until you reauthenticate.'
|
||||
})) {
|
||||
return;
|
||||
}
|
||||
try {
|
||||
|
|
@ -3075,7 +3077,7 @@ async function disconnectSpotify() {
|
|||
const response = await fetch('/api/spotify/disconnect', { method: 'POST' });
|
||||
const data = await response.json();
|
||||
if (data.success) {
|
||||
showToast(`Spotify disconnected. Now using ${fallbackName}.`, 'success');
|
||||
showToast(data.message || 'Spotify disconnected.', 'success');
|
||||
// Immediately refresh status to update UI
|
||||
await fetchAndUpdateServiceStatus();
|
||||
} else {
|
||||
|
|
@ -3089,29 +3091,6 @@ async function disconnectSpotify() {
|
|||
}
|
||||
}
|
||||
|
||||
async function clearSpotifyCacheAndFallback() {
|
||||
const fallbackName = currentMusicSourceName !== 'Spotify' ? currentMusicSourceName : 'the configured fallback source';
|
||||
if (!await showConfirmDialog({
|
||||
title: 'Clear Spotify Cache',
|
||||
message: `This will clear the Spotify token cache and switch metadata to ${fallbackName}. You can re-authenticate later.`
|
||||
})) return;
|
||||
try {
|
||||
showLoadingOverlay('Clearing Spotify cache...');
|
||||
const response = await fetch('/api/spotify/disconnect', { method: 'POST' });
|
||||
const data = await response.json();
|
||||
if (data.success) {
|
||||
showToast(data.message || `Switched to ${fallbackName}`, 'success');
|
||||
await fetchAndUpdateServiceStatus();
|
||||
} else {
|
||||
showToast(`Failed: ${data.error}`, 'error');
|
||||
}
|
||||
} catch (error) {
|
||||
showToast('Failed to clear Spotify cache', 'error');
|
||||
} finally {
|
||||
hideLoadingOverlay();
|
||||
}
|
||||
}
|
||||
|
||||
// ── Spotify Rate Limit Handling ───────────────────────────────────────────
|
||||
let _spotifyRateLimitShown = false;
|
||||
let _spotifyInCooldown = false;
|
||||
|
|
@ -3198,7 +3177,7 @@ async function disconnectSpotifyFromRateLimit() {
|
|||
const data = await response.json();
|
||||
if (data.success) {
|
||||
_spotifyRateLimitShown = false;
|
||||
showToast(`Spotify disconnected. Now using ${currentMusicSourceName}.`, 'success');
|
||||
showToast(data.message || 'Spotify disconnected.', 'success');
|
||||
await fetchAndUpdateServiceStatus();
|
||||
if (currentPage === 'discover') {
|
||||
loadDiscoverPage();
|
||||
|
|
@ -3879,4 +3858,3 @@ function togglePathLock(pathType, btn) {
|
|||
|
||||
|
||||
// ===============================
|
||||
|
||||
|
|
|
|||
|
|
@ -3194,10 +3194,15 @@ function updateServiceStatus(service, statusData) {
|
|||
currentMusicSourceName = sourceName;
|
||||
}
|
||||
|
||||
// Show/hide Spotify disconnect button based on connection state
|
||||
// Keep the Spotify action buttons aligned with the actual auth session.
|
||||
const spotifySessionActive = statusData.authenticated === true || (statusData.authenticated === undefined && statusData.source === 'spotify');
|
||||
const authBtn = document.querySelector('button[onclick="authenticateSpotify()"]');
|
||||
const disconnectBtn = document.getElementById('spotify-disconnect-btn');
|
||||
if (authBtn) {
|
||||
authBtn.style.display = spotifySessionActive ? 'none' : '';
|
||||
}
|
||||
if (disconnectBtn) {
|
||||
disconnectBtn.style.display = statusData.source === 'spotify' ? '' : 'none';
|
||||
disconnectBtn.style.display = spotifySessionActive ? '' : 'none';
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue