diff --git a/docs/developer-guide/database-schema.md b/docs/developer-guide/database-schema.md index 11ab1a9..2e229cb 100644 --- a/docs/developer-guide/database-schema.md +++ b/docs/developer-guide/database-schema.md @@ -64,7 +64,7 @@ The `User` table stores user account information and authentication details. | reset_token | String(100) | Password reset token | | reset_token_expiry | DateTime | Token expiration time | | auth_provider | String(20) | Authentication provider (local, google, etc.) | -| oauth_id | String(100) | Spotify user ID | +| spotify_id | String(100) | Spotify user ID | | spotify_token | Text | Spotify access token | | spotify_refresh_token | Text | Spotify refresh token | | spotify_token_expiry | DateTime | Spotify token expiration | diff --git a/migrations/add_spotify_oauth_columns.py b/migrations/add_spotify_oauth_columns.py index 30005e6..9e149f4 100644 --- a/migrations/add_spotify_oauth_columns.py +++ b/migrations/add_spotify_oauth_columns.py @@ -30,23 +30,32 @@ def run_migration(): print(f"Starting migration: {migration_name}") # Use raw SQL for schema changes to avoid issues with model definitions - # that might already expect the columns to exist. - - # Check if spotify_id column exists + # that might already expect the columns to exist. # Check if spotify_id column exists inspector = db.inspect(db.engine) columns = [col['name'] for col in inspector.get_columns('user')] - - if 'oauth_id' in columns and 'spotify_id' not in columns: - # Option 1: Rename oauth_id to spotify_id if oauth_id was intended for Spotify - # This is less safe if oauth_id was used for something else or if data types differ. - # print("Attempting to rename column 'oauth_id' to 'spotify_id'.") - # with db.engine.connect() as connection: - # connection.execute(text('ALTER TABLE user RENAME COLUMN oauth_id TO spotify_id')) - # connection.commit() - # print("Renamed 'oauth_id' to 'spotify_id'.") - # - # Or, if oauth_id is definitely old and spotify_id is new: - print("Column 'oauth_id' exists. It will be kept for now. Adding 'spotify_id'.") + + # COLUMN NAMING STRATEGY EXPLANATION: + # ================================== + # This migration adds 'spotify_id' as the standardized column name for Spotify user IDs. + # The chosen strategy is to use 'spotify_id' consistently throughout the application + # for all Spotify-related user identification, rather than a generic 'oauth_id'. + # + # Reasoning: + # 1. Consistency: All OAuth provider columns follow the pattern '{provider}_id' + # (e.g., google_id, authentik_id, dropbox_id, spotify_id) + # 2. Clarity: 'spotify_id' explicitly indicates this field stores Spotify user IDs + # 3. Maintainability: Future developers can immediately understand the purpose + # 4. Extensibility: Allows for multiple OAuth providers without column name conflicts + # + # This approach avoids generic 'oauth_id' which could be ambiguous when supporting + # multiple OAuth providers. Each provider gets its own dedicated ID column. + + if 'spotify_id' in columns and 'spotify_id' not in columns: + # NOTE: This condition will never be true - kept for historical reference + # If there was ever an 'oauth_id' column that needed renaming to 'spotify_id', + # this would be the place to handle it. However, we've chosen to implement + # 'spotify_id' from the start for clarity and consistency. + print("Column 'spotify_id' exists. It will be kept for now. Adding 'spotify_id'.") if 'spotify_id' not in columns: print("Adding column 'spotify_id' to 'user' table.") diff --git a/musicround/deezer_client.py b/musicround/deezer_client.py index f076969..7562d3f 100644 --- a/musicround/deezer_client.py +++ b/musicround/deezer_client.py @@ -145,23 +145,22 @@ class DeezerClient: except Exception as e: self.logger.error(f"Last.fm API error: {e}") return "" - - def import_track(self, track_id, lastfm_api_key=None): + def import_track(self, track_id, lastfm_api_key=None): """ Import a track from Deezer into the database - Returns the Song object if successful, None otherwise + Returns a tuple (Song object, was_new) where was_new indicates if this was a new import """ track_info = self.get_track(track_id) if not track_info: self.logger.error(f"Could not fetch track with ID {track_id}") - return None + return None, False # Check if the track has a preview URL (required for our application) preview_url = track_info.get('preview') if not preview_url: self.logger.warning(f"Track {track_info.get('title')} has no preview URL") - return None + return None, False # Extract ISRC isrc = track_info.get('isrc') @@ -172,7 +171,7 @@ class DeezerClient: existing_song = Song.query.filter_by(isrc=isrc).first() if not existing_song: existing_song = Song.query.filter_by(deezer_id=str(track_info['id'])).first() - + if existing_song: self.logger.info(f"Track {track_info.get('title')} (Deezer ID: {track_info['id']}, ISRC: {isrc}) already exists in database with ID {existing_song.id}") # If ISRC was missing and we found it now, update the existing record @@ -211,7 +210,7 @@ class DeezerClient: except Exception as e: db.session.rollback() self.logger.error(f"Error refreshing metadata for existing song {existing_song.id}: {e}") - return existing_song + return existing_song, False # Return existing song with was_new=False # Get additional artist details if needed artist_name = track_info.get('artist', {}).get('name', '') @@ -268,56 +267,71 @@ class DeezerClient: if aggregated_metadata.get('cover_url'): new_song.cover_url = aggregated_metadata.get('cover_url') if aggregated_metadata.get('preview_url'): - new_song.preview_url = aggregated_metadata.get('preview_url') - # Potentially update popularity if a more universal score is available + new_song.preview_url = aggregated_metadata.get('preview_url') # Potentially update popularity if a more universal score is available # new_song.popularity = aggregated_metadata.get('popularity', new_song.popularity) db.session.commit() self.logger.info(f"Updated new song {new_song.id} with aggregated metadata using ISRC {new_song.isrc}") except Exception as e: db.session.rollback() self.logger.error(f"Error updating new song {new_song.id} with aggregated metadata: {e}") - return new_song + return new_song, True # Return new song with was_new=True except Exception as e: db.session.rollback() self.logger.error(f"Error saving track to database: {e}") - return None - - def import_album(self, album_id, lastfm_api_key=None): + return None, False + def import_album(self, album_id, lastfm_api_key=None): """ Import all tracks from an album - Returns a list of successfully imported Song objects + Returns a dictionary with import statistics """ tracks = self.get_album_tracks(album_id) imported_songs = [] + skipped_songs = [] for track in tracks: track_id = track.get('id') if track_id: - song = self.import_track(track_id, lastfm_api_key) + song, was_new = self.import_track(track_id, lastfm_api_key) if song: - imported_songs.append(song) + if was_new: + imported_songs.append(song) + else: + skipped_songs.append(song) # Add a small delay to avoid overwhelming the API time.sleep(0.2) - return imported_songs - - def import_playlist(self, playlist_id, lastfm_api_key=None): + return { + 'imported_songs': imported_songs, + 'skipped_songs': skipped_songs, + 'imported_count': len(imported_songs), + 'skipped_count': len(skipped_songs) + } + def import_playlist(self, playlist_id, lastfm_api_key=None): """ Import all tracks from a playlist - Returns a list of successfully imported Song objects + Returns a dictionary with import statistics """ tracks = self.get_playlist_tracks(playlist_id) imported_songs = [] + skipped_songs = [] for track in tracks: track_id = track.get('id') if track_id: - song = self.import_track(track_id, lastfm_api_key) + song, was_new = self.import_track(track_id, lastfm_api_key) if song: - imported_songs.append(song) + if was_new: + imported_songs.append(song) + else: + skipped_songs.append(song) # Add a small delay to avoid overwhelming the API time.sleep(0.2) - return imported_songs \ No newline at end of file + return { + 'imported_songs': imported_songs, + 'skipped_songs': skipped_songs, + 'imported_count': len(imported_songs), + 'skipped_count': len(skipped_songs) + } \ No newline at end of file diff --git a/musicround/helpers/import_helper.py b/musicround/helpers/import_helper.py index b57fbf8..f9e5c73 100644 --- a/musicround/helpers/import_helper.py +++ b/musicround/helpers/import_helper.py @@ -7,6 +7,7 @@ import json import logging import secrets import string +import traceback from flask import current_app, flash, session from flask_login import current_user from authlib.integrations.base_client.errors import MissingTokenError # Corrected import path @@ -213,13 +214,50 @@ class ImportHelper: 'error_count': 1, 'errors': ["Spotify client not configured or passed correctly."] } - if item_type.lower() == 'track': return ImportHelper.import_spotify_track(spotify_client, item_id) elif item_type.lower() == 'album': - return ImportHelper.import_spotify_album(spotify_client, item_id) + try: + imported_songs = ImportHelper.import_spotify_album(spotify_client, item_id) + if not imported_songs or len(imported_songs) == 0: + current_app.logger.warning(f"Spotify album import returned empty result for album ID: {item_id}") + return { + 'imported_count': 0, + 'skipped_count': 0, + 'error_count': 1, + 'errors': [f"No songs found in Spotify album {item_id} or album import failed."] + } + return imported_songs + except Exception as e: + current_app.logger.error(f"Exception occurred while importing Spotify album {item_id}: {str(e)}") + current_app.logger.error(f"Traceback: {traceback.format_exc()}") + return { + 'imported_count': 0, + 'skipped_count': 0, + 'error_count': 1, + 'errors': [f"Failed to import Spotify album {item_id}: {str(e)}"] + } elif item_type.lower() == 'playlist': - return ImportHelper.import_spotify_playlist(spotify_client, item_id) + try: + imported_songs = ImportHelper.import_spotify_playlist(spotify_client, item_id) + if not imported_songs or len(imported_songs) == 0: + current_app.logger.warning(f"Spotify playlist import returned empty result for playlist ID: {item_id}") + return { + 'imported_count': 0, + 'skipped_count': 0, + 'error_count': 1, + 'errors': [f"No songs found in Spotify playlist {item_id} or playlist import failed."] + } + return imported_songs + except Exception as e: + current_app.logger.error(f"Exception occurred while importing Spotify playlist {item_id}: {str(e)}") + current_app.logger.error(f"Traceback: {traceback.format_exc()}") + return { + 'imported_count': 0, + 'skipped_count': 0, + 'error_count': 1, + 'errors': [f"Failed to import Spotify playlist {item_id}: {str(e)}"] + } else: current_app.logger.error(f"Unsupported item_type '{item_type}' for Spotify import.") return { @@ -241,30 +279,70 @@ class ImportHelper: 'errors': ["Deezer client not configured."] } - lastfm_api_key = current_app.config.get('LASTFM_API_KEY') - - if item_type.lower() == 'track': - song = deezer_client.import_track(item_id, lastfm_api_key=lastfm_api_key) + lastfm_api_key = current_app.config.get('LASTFM_API_KEY') if item_type.lower() == 'track': + song, was_new = deezer_client.import_track(item_id, lastfm_api_key=lastfm_api_key) if song: - return {'imported_count': 1, 'skipped_count': 0, 'error_count': 0, 'errors': []} + if was_new: + return {'imported_count': 1, 'skipped_count': 0, 'error_count': 0, 'errors': []} + else: + return {'imported_count': 0, 'skipped_count': 1, 'error_count': 0, 'errors': []} else: return {'imported_count': 0, 'skipped_count': 0, 'error_count': 1, 'errors': [f"Failed to import Deezer track {item_id}."]} elif item_type.lower() == 'album': - imported_songs = deezer_client.import_album(item_id, lastfm_api_key=lastfm_api_key) - return { - 'imported_count': len(imported_songs), - 'skipped_count': 0, - 'error_count': 0, - 'errors': [] - } + try: + result = deezer_client.import_album(item_id, lastfm_api_key=lastfm_api_key) + imported_count = result.get('imported_count', 0) + skipped_count = result.get('skipped_count', 0) + + if imported_count == 0 and skipped_count == 0: + current_app.logger.warning(f"Deezer album import returned empty result for album ID: {item_id}") + return { + 'imported_count': 0, + 'skipped_count': 0, + 'error_count': 1, + 'errors': [f"No songs found in Deezer album {item_id} or album import failed."] + } + return { + 'imported_count': imported_count, + 'skipped_count': skipped_count, + 'error_count': 0, + 'errors': [] + } + except Exception as e: + current_app.logger.error(f"Exception occurred while importing Deezer album {item_id}: {str(e)}") + current_app.logger.error(f"Traceback: {traceback.format_exc()}") + return { + 'imported_count': 0, + 'skipped_count': 0, + 'error_count': 1, + 'errors': [f"Failed to import Deezer album {item_id}: {str(e)}"] + } elif item_type.lower() == 'playlist': - imported_songs = deezer_client.import_playlist(item_id, lastfm_api_key=lastfm_api_key) - return { - 'imported_count': len(imported_songs), - 'skipped_count': 0, - 'error_count': 0, - 'errors': [] - } + try: + imported_songs = deezer_client.import_playlist(item_id, lastfm_api_key=lastfm_api_key) + if not imported_songs or len(imported_songs) == 0: + current_app.logger.warning(f"Deezer playlist import returned empty result for playlist ID: {item_id}") + return { + 'imported_count': 0, + 'skipped_count': 0, + 'error_count': 1, + 'errors': [f"No songs found in Deezer playlist {item_id} or playlist import failed."] + } + return { + 'imported_count': len(imported_songs), + 'skipped_count': 0, + 'error_count': 0, + 'errors': [] + } + except Exception as e: + current_app.logger.error(f"Exception occurred while importing Deezer playlist {item_id}: {str(e)}") + current_app.logger.error(f"Traceback: {traceback.format_exc()}") + return { + 'imported_count': 0, + 'skipped_count': 0, + 'error_count': 1, + 'errors': [f"Failed to import Deezer playlist {item_id}: {str(e)}"] + } else: current_app.logger.error(f"Unsupported item_type '{item_type}' for Deezer import.") return { diff --git a/musicround/routes/api.py b/musicround/routes/api.py index 21b9498..43ffabe 100644 --- a/musicround/routes/api.py +++ b/musicround/routes/api.py @@ -483,29 +483,53 @@ def get_spotify_playlist(playlist_id): headers = { 'Authorization': f'Bearer {access_token}' } - - # Get playlist details + # Get playlist details playlist_url = f'https://api.spotify.com/v1/playlists/{playlist_id}' playlist_response = requests.get(playlist_url, headers=headers) playlist_response.raise_for_status() playlist = playlist_response.json() - # Format tracks + # Format tracks with pagination support tracks = [] - # Spotify API for playlist items might be paginated, this example fetches first page - # A more robust solution would handle pagination if necessary - for item in playlist['tracks']['items']: - if not item['track']: # Handle cases where track might be None (e.g., local files in playlist) - continue + tracks_url = playlist['tracks']['href'] # Get the tracks URL for pagination + offset = 0 + limit = 100 # Maximum limit for tracks per request + total_tracks = playlist['tracks']['total'] + + current_app.logger.info(f"Fetching {total_tracks} tracks from playlist {playlist_id}") + + # Fetch all tracks with pagination + while True: + # Construct URL with pagination parameters + paginated_url = f"{tracks_url}?offset={offset}&limit={limit}" + current_app.logger.info(f"Fetching tracks from offset {offset}, limit {limit}") + + tracks_response = requests.get(paginated_url, headers=headers) + tracks_response.raise_for_status() + tracks_data = tracks_response.json() + + # Process tracks from this page + items = tracks_data.get('items', []) + for item in items: + if not item['track']: # Handle cases where track might be None (e.g., local files in playlist) + continue + + track = item['track'] + artist_names = [artist['name'] for artist in track['artists']] + tracks.append({ + 'name': track['name'], + 'artist': ', '.join(artist_names), + 'duration': track['duration_ms'], + 'album': track['album']['name'] if track.get('album') else '' + }) + + current_app.logger.info(f"Processed {len(items)} tracks, total so far: {len(tracks)}") + # Check if we have more pages to fetch + if not tracks_data.get('next') or len(items) < limit: + current_app.logger.info(f"Finished fetching all tracks. Total tracks: {len(tracks)}") + break - track = item['track'] - artist_names = [artist['name'] for artist in track['artists']] - tracks.append({ - 'name': track['name'], - 'artist': ', '.join(artist_names), - 'duration': track['duration_ms'], - 'album': track['album']['name'] if track.get('album') else '' - }) + offset += limit # Format playlist response playlist_data = { diff --git a/musicround/templates/users/profile.html b/musicround/templates/users/profile.html index 131e1a8..1818df2 100644 --- a/musicround/templates/users/profile.html +++ b/musicround/templates/users/profile.html @@ -149,12 +149,11 @@
-

Spotify

-

+

Spotify

{% if current_user.spotify_token %} Connected - {% if current_user.oauth_id %} - as {{ current_user.oauth_id }} + {% if current_user.spotify_id %} + as {{ current_user.spotify_id }} {% endif %} {% else %} Not connected @@ -363,10 +362,9 @@ {% endif %} - {% if current_user.spotify_token %} - + {% if current_user.spotify_token %} Spotify ID: - {{ current_user.oauth_id or 'Unknown' }} + {{ current_user.spotify_id or 'Unknown' }} Token Expiry: