From 30139bcdb4aae7375e8643726ca904d52ff4b2ba Mon Sep 17 00:00:00 2001 From: Cristan Beskid <596554+sferra@users.noreply.github.com> Date: Sat, 17 Feb 2024 00:54:42 +0100 Subject: [PATCH 1/5] Fix artist search when lastfm information isn't available --- packages/core/src/plugins/meta/discogs.ts | 41 +++++++++++++++-------- 1 file changed, 27 insertions(+), 14 deletions(-) diff --git a/packages/core/src/plugins/meta/discogs.ts b/packages/core/src/plugins/meta/discogs.ts index 3c76153a27..2d60068454 100644 --- a/packages/core/src/plugins/meta/discogs.ts +++ b/packages/core/src/plugins/meta/discogs.ts @@ -16,7 +16,6 @@ import { import { DiscogsReleaseSearchResponse, DiscogsArtistSearchResponse, - DiscogsArtistReleasesSearchResponse, DiscogsArtistInfo, DiscogsArtistSearchResult, DiscogsReleaseSearchResult, @@ -175,29 +174,19 @@ class DiscogsMetaProvider extends MetaProvider { const lastFmInfo: LastFmArtistInfo = (await (await this.lastfm.getArtistInfo(discogsInfo.name)).json()).artist; const lastFmTopTracks: LastfmTopTracks = (await (await this.lastfm.getArtistTopTracks(discogsInfo.name)).json()).toptracks; - const coverImage = this.getCoverImage(discogsInfo); - - const spotifyToken = await getToken(); - const similarArtists = await Promise.all(take(lastFmInfo.similar.artist, 5).map(async artist => { - const similarArtist = await searchArtists(spotifyToken, artist.name); - - return { - name: artist.name, - thumbnail: similarArtist?.images[0]?.url - }; - })); + const similarArtists = await this.getSimilarArtists(lastFmInfo); return { id: discogsInfo.id, name: discogsInfo.name, description: _.get(lastFmInfo, 'bio.summary'), tags: _.map(_.get(lastFmInfo, 'tags.tag'), 'name'), - onTour: lastFmInfo.ontour === '1', + onTour: this.isArtistOnTour(lastFmInfo), coverImage, thumb: coverImage, images: _.map(discogsInfo.images, 'resource_url'), - topTracks: _.map(lastFmTopTracks.track, (track: LastfmTrack) => ({ + topTracks: _.map(lastFmTopTracks?.track, (track: LastfmTrack) => ({ name: track.name, title: track.name, thumb: coverImage, @@ -210,6 +199,30 @@ class DiscogsMetaProvider extends MetaProvider { }; } + async getSimilarArtists(artistInfo: LastFmArtistInfo | undefined) { + if (!artistInfo) { + return []; + } + const spotifyToken = await getToken(); + return await Promise.all( + take(artistInfo.similar.artist, 5) + .map(async artist => { + const similarArtist = await searchArtists(spotifyToken, artist.name); + return { + name: artist.name, + thumbnail: similarArtist?.images[0]?.url + }; + }) + ); + } + + isArtistOnTour(artistInfo: LastFmArtistInfo | undefined): boolean { + if (!artistInfo) { + return false; + } + return artistInfo?.ontour === '1'; + } + async fetchArtistDetailsByName(artistName: string): Promise { const artistSearch = await (await Discogs.search(artistName, 'artist')).json(); const artist: DiscogsArtistSearchResult = _.head(artistSearch.results); From d00639a6cc8c99c133f606a3b0b89bc2b9301781 Mon Sep 17 00:00:00 2001 From: Cristan Beskid <596554+sferra@users.noreply.github.com> Date: Sat, 17 Feb 2024 01:04:08 +0100 Subject: [PATCH 2/5] Remove obsolete check --- packages/core/src/plugins/meta/discogs.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/core/src/plugins/meta/discogs.ts b/packages/core/src/plugins/meta/discogs.ts index 2d60068454..3ec64151e5 100644 --- a/packages/core/src/plugins/meta/discogs.ts +++ b/packages/core/src/plugins/meta/discogs.ts @@ -217,9 +217,6 @@ class DiscogsMetaProvider extends MetaProvider { } isArtistOnTour(artistInfo: LastFmArtistInfo | undefined): boolean { - if (!artistInfo) { - return false; - } return artistInfo?.ontour === '1'; } From f6ff1afc302d73d577c0bae6e5bfd7e518317881 Mon Sep 17 00:00:00 2001 From: Cristan Beskid <596554+sferra@users.noreply.github.com> Date: Mon, 19 Feb 2024 00:25:12 +0100 Subject: [PATCH 3/5] Refactor to allow testing and add tests --- .../core/src/plugins/meta/discogs.test.ts | 164 ++++++++++++++++++ packages/core/src/plugins/meta/discogs.ts | 58 +++++-- packages/core/src/rest/Spotify.ts | 11 +- 3 files changed, 212 insertions(+), 21 deletions(-) create mode 100644 packages/core/src/plugins/meta/discogs.test.ts diff --git a/packages/core/src/plugins/meta/discogs.test.ts b/packages/core/src/plugins/meta/discogs.test.ts new file mode 100644 index 0000000000..f149fbafec --- /dev/null +++ b/packages/core/src/plugins/meta/discogs.test.ts @@ -0,0 +1,164 @@ +import DiscogsMetaProvider from './discogs'; +import { SimilarArtist } from '../plugins.types'; +import * as SpotifyApi from '../../rest/Spotify'; + +describe('Tests for DiscogsMetaProvider', () => { + const provider = new DiscogsMetaProvider(); + + describe('Verify if an artist is on tour', () => { + + test('Should return false if the artist info is missing', () => { + expect(provider.isArtistOnTour(undefined)).toBeFalsy(); + }); + + test('Should return false if the \'ontour\' flag is not \'1\'', () => { + const artistInfo: any = { + ontour: 'something else than 1' + }; + expect(provider.isArtistOnTour(artistInfo)).toBeFalsy(); + }); + + test('Should return false if the \'ontour\' flag is \'1\'', () => { + const artistInfo: any = { + ontour: '1' + }; + expect(provider.isArtistOnTour(artistInfo)).toBeTruthy(); + }); + }); + + describe('Get similar artists', () => { + const spotifyToken = 'spotify token'; + const getToken = jest.spyOn(SpotifyApi, 'getToken'); + const fetchTopSimilarArtistsFromSpotify = jest.spyOn(provider, 'fetchTopSimilarArtistsFromSpotify'); + + afterEach(() => { + getToken.mockReset(); + fetchTopSimilarArtistsFromSpotify.mockReset(); + }); + + afterAll(() => { + getToken.mockRestore(); + fetchTopSimilarArtistsFromSpotify.mockRestore(); + }); + + test('Should return an empty result set if the artist info is missing', async () => { + expect(await provider.getSimilarArtists({ + similar: { + artist: null + } + } as any)).toEqual([]); + }); + + test('Should fetch similar artist', async () => { + const similarArtistsOnLastFm = [{ + name: 'Similar Artist on LastFm' + }]; + const artistInfoFromLastFm = { + name: 'Artist Name', + similar: { + artist: similarArtistsOnLastFm + } + } as any; + + getToken.mockResolvedValueOnce(spotifyToken); + + const similarArtist: SimilarArtist = { + name: 'Similar Artist Name', + thumbnail: 'the thumbnail' + }; + fetchTopSimilarArtistsFromSpotify.mockImplementationOnce((artists, token) => { + expect(token).toEqual(spotifyToken); + expect(artists).toEqual(similarArtistsOnLastFm); + return Promise.resolve([similarArtist]); + }); + expect(await provider.getSimilarArtists(artistInfoFromLastFm)).toEqual([similarArtist]); + }); + + test('Should return empty result set if fetching the Spotify token fails', async () => { + getToken.mockRejectedValueOnce('Failed to fetch Spotify token'); + expect(await provider.getSimilarArtists({ + similar: { + artist: [] + } + } as any)).toEqual([]); + }); + + test('Should return empty result set if fetching the artists from Spotify fails', async () => { + getToken.mockResolvedValueOnce(spotifyToken); + fetchTopSimilarArtistsFromSpotify.mockRejectedValueOnce('Failed to fetch similar artists'); + expect(await provider.getSimilarArtists({ + similar: { + artist: [] + } + } as any)).toEqual([]); + }); + }); + + describe('Fetch top similar artists from Spotify', () => { + + test('Should fetch similar artists from Spotify', async () => { + const fetchSimilarArtistFromSpotify = jest.spyOn(provider, 'fetchSimilarArtistFromSpotify') + .mockImplementation((artistName, token) => { + return Promise.resolve({ + name: `Spotify: ${artistName}` + } as any); + }); + + const spotifyToken = 'Spotify token'; + const artistsFromLastfm = [ + { name: null }, + { name: 'Artist 1' }, + { name: 'Artist 2' }, + { name: 'Artist 3' }, + { name: null }, + { name: 'Artist 4' }, + { name: 'Artist 5' }, + { name: 'Artist 6' } + ] as any; + + expect(await provider.fetchTopSimilarArtistsFromSpotify(artistsFromLastfm, spotifyToken)) + .toEqual([ + { 'name': 'Spotify: Artist 1' }, + { 'name': 'Spotify: Artist 2' }, + { 'name': 'Spotify: Artist 3' }, + { 'name': 'Spotify: Artist 4' }, + { 'name': 'Spotify: Artist 5' } + ]); + expect(fetchSimilarArtistFromSpotify).toHaveBeenCalledTimes(5); + fetchSimilarArtistFromSpotify.mockRestore(); + }); + }); + + describe('Fetch similar artist from Spotify', () => { + const searchArtists = jest.spyOn(SpotifyApi, 'searchArtists'); + + afterEach(() => { + searchArtists.mockReset(); + }); + + afterAll(() => { + searchArtists.mockRestore(); + }); + + test('Should fetch similar artist', async () => { + searchArtists.mockResolvedValueOnce({ + images: [{ url: 'the thumbnail URL' }] + } as any); + expect(await provider.fetchSimilarArtistFromSpotify('Artist Name', 'Spotify token')) + .toEqual({ + name: 'Artist Name', + thumbnail: 'the thumbnail URL' + }); + }); + + test('Should return shallow similar artist if fetching fails', async () => { + searchArtists.mockRejectedValueOnce('Failed to fetch artist'); + expect(await provider.fetchSimilarArtistFromSpotify('Artist Name', 'Spotify token')) + .toEqual({ + name: 'Artist Name', + thumbnail: null + }); + }); + }); + +}); diff --git a/packages/core/src/plugins/meta/discogs.ts b/packages/core/src/plugins/meta/discogs.ts index 3ec64151e5..99376f4873 100644 --- a/packages/core/src/plugins/meta/discogs.ts +++ b/packages/core/src/plugins/meta/discogs.ts @@ -1,4 +1,4 @@ -import _, { take } from 'lodash'; +import _ from 'lodash'; import MetaProvider from '../metaProvider'; import * as Discogs from '../../rest/Discogs'; @@ -11,7 +11,8 @@ import { SearchResultsSource, ArtistDetails, AlbumDetails, - AlbumType + AlbumType, + SimilarArtist } from '../plugins.types'; import { DiscogsReleaseSearchResponse, @@ -23,9 +24,10 @@ import { DiscogsReleaseInfo, DiscogsTrack } from '../../rest/Discogs.types'; -import { LastFmArtistInfo, LastfmTopTracks, LastfmTrack } from '../../rest/Lastfm.types'; +import { LastFmArtistInfo, LastfmTopTracks, LastfmTrack, LastfmArtistShort } from '../../rest/Lastfm.types'; import { cleanName } from '../../structs/Artist'; import { getToken, searchArtists } from '../../rest/Spotify'; +import logger from 'electron-timber'; class DiscogsMetaProvider extends MetaProvider { lastfm: LastFmApi; @@ -199,23 +201,47 @@ class DiscogsMetaProvider extends MetaProvider { }; } - async getSimilarArtists(artistInfo: LastFmArtistInfo | undefined) { - if (!artistInfo) { - return []; + async getSimilarArtists(artist: LastFmArtistInfo | undefined): Promise { + if (!artist?.similar?.artist) { + return Promise.resolve([]); } - const spotifyToken = await getToken(); - return await Promise.all( - take(artistInfo.similar.artist, 5) - .map(async artist => { - const similarArtist = await searchArtists(spotifyToken, artist.name); - return { - name: artist.name, - thumbnail: similarArtist?.images[0]?.url - }; - }) + const similarArtists = artist.similar.artist; + return getToken() + .then(spotifyToken => this.fetchTopSimilarArtistsFromSpotify(similarArtists, spotifyToken)) + .catch(error => { + logger.error(`Failed to fetch similar artists for '${artist.name}'`); + logger.error(error); + return []; + }); + } + + async fetchTopSimilarArtistsFromSpotify(artists: LastfmArtistShort[], spotifyToken: string) { + return Promise.all( + artists + .filter(artist => artist?.name) + .slice(0, 5) + .map(artist => this.fetchSimilarArtistFromSpotify(artist.name, spotifyToken)) ); } + async fetchSimilarArtistFromSpotify(artistName: string, spotifyToken: string): Promise { + return searchArtists(spotifyToken, artistName) + .then(spotifyArtist => { + return { + name: artistName, + thumbnail: spotifyArtist?.images[0]?.url + }; + }) + .catch(error => { + logger.error(`Failed to fetch artist from Spotify: '${artistName}'`); + logger.error(error); + return { + name: artistName, + thumbnail: null + }; + }); + } + isArtistOnTour(artistInfo: LastFmArtistInfo | undefined): boolean { return artistInfo?.ontour === '1'; } diff --git a/packages/core/src/rest/Spotify.ts b/packages/core/src/rest/Spotify.ts index 5b493d4252..cc17ca23fd 100644 --- a/packages/core/src/rest/Spotify.ts +++ b/packages/core/src/rest/Spotify.ts @@ -1,11 +1,12 @@ const SPOTIFY_API_OPEN_URL = 'https://open.spotify.com'; const SPOTIFY_API_URL = 'https://api.spotify.com/v1'; -type SpotifyArtist = { - images: { - height: number; - url: string; - }[]; +export type SpotifyArtist = { + name: string; + images: { + height: number; + url: string; + }[]; } export const getToken = async (): Promise => { From 36f708134f0df7c5b57742f3284ec3f893182678 Mon Sep 17 00:00:00 2001 From: Cristan Beskid <596554+sferra@users.noreply.github.com> Date: Mon, 19 Feb 2024 18:46:47 +0100 Subject: [PATCH 4/5] Cast test data to explicit types --- packages/core/src/plugins/meta/discogs.test.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/core/src/plugins/meta/discogs.test.ts b/packages/core/src/plugins/meta/discogs.test.ts index f149fbafec..f12727aaf1 100644 --- a/packages/core/src/plugins/meta/discogs.test.ts +++ b/packages/core/src/plugins/meta/discogs.test.ts @@ -1,6 +1,8 @@ import DiscogsMetaProvider from './discogs'; import { SimilarArtist } from '../plugins.types'; import * as SpotifyApi from '../../rest/Spotify'; +import { SpotifyArtist } from '../../rest/Spotify'; +import { LastFmArtistInfo, LastfmArtistShort } from '../../rest/Lastfm.types'; describe('Tests for DiscogsMetaProvider', () => { const provider = new DiscogsMetaProvider(); @@ -46,7 +48,7 @@ describe('Tests for DiscogsMetaProvider', () => { similar: { artist: null } - } as any)).toEqual([]); + } as LastFmArtistInfo)).toEqual([]); }); test('Should fetch similar artist', async () => { @@ -58,7 +60,7 @@ describe('Tests for DiscogsMetaProvider', () => { similar: { artist: similarArtistsOnLastFm } - } as any; + } as LastFmArtistInfo; getToken.mockResolvedValueOnce(spotifyToken); @@ -80,7 +82,7 @@ describe('Tests for DiscogsMetaProvider', () => { similar: { artist: [] } - } as any)).toEqual([]); + } as LastFmArtistInfo)).toEqual([]); }); test('Should return empty result set if fetching the artists from Spotify fails', async () => { @@ -90,7 +92,7 @@ describe('Tests for DiscogsMetaProvider', () => { similar: { artist: [] } - } as any)).toEqual([]); + } as LastFmArtistInfo)).toEqual([]); }); }); @@ -101,7 +103,7 @@ describe('Tests for DiscogsMetaProvider', () => { .mockImplementation((artistName, token) => { return Promise.resolve({ name: `Spotify: ${artistName}` - } as any); + } as SimilarArtist); }); const spotifyToken = 'Spotify token'; @@ -114,7 +116,7 @@ describe('Tests for DiscogsMetaProvider', () => { { name: 'Artist 4' }, { name: 'Artist 5' }, { name: 'Artist 6' } - ] as any; + ] as LastfmArtistShort[]; expect(await provider.fetchTopSimilarArtistsFromSpotify(artistsFromLastfm, spotifyToken)) .toEqual([ @@ -143,7 +145,7 @@ describe('Tests for DiscogsMetaProvider', () => { test('Should fetch similar artist', async () => { searchArtists.mockResolvedValueOnce({ images: [{ url: 'the thumbnail URL' }] - } as any); + } as SpotifyArtist); expect(await provider.fetchSimilarArtistFromSpotify('Artist Name', 'Spotify token')) .toEqual({ name: 'Artist Name', From 26392a81a57c7f5619606bad0617f3c7bf5b5200 Mon Sep 17 00:00:00 2001 From: Cristan Beskid <596554+sferra@users.noreply.github.com> Date: Fri, 23 Feb 2024 18:12:38 +0100 Subject: [PATCH 5/5] Change then/catch sequence to try/await/catch As requested in the review. --- packages/core/src/plugins/meta/discogs.ts | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/packages/core/src/plugins/meta/discogs.ts b/packages/core/src/plugins/meta/discogs.ts index 99376f4873..5a796392b6 100644 --- a/packages/core/src/plugins/meta/discogs.ts +++ b/packages/core/src/plugins/meta/discogs.ts @@ -203,16 +203,17 @@ class DiscogsMetaProvider extends MetaProvider { async getSimilarArtists(artist: LastFmArtistInfo | undefined): Promise { if (!artist?.similar?.artist) { - return Promise.resolve([]); + return []; } const similarArtists = artist.similar.artist; - return getToken() - .then(spotifyToken => this.fetchTopSimilarArtistsFromSpotify(similarArtists, spotifyToken)) - .catch(error => { - logger.error(`Failed to fetch similar artists for '${artist.name}'`); - logger.error(error); - return []; - }); + try { + const spotifyToken = await getToken(); + return await this.fetchTopSimilarArtistsFromSpotify(similarArtists, spotifyToken); + } catch (error) { + logger.error(`Failed to fetch similar artists for '${artist.name}'`); + logger.error(error); + } + return []; } async fetchTopSimilarArtistsFromSpotify(artists: LastfmArtistShort[], spotifyToken: string) {