New: Refactor metadata update

src/NzbDrone.Core.Test/MediaFiles/TrackImport/Identification/IdentificationServiceFixture.cs

@@ -61,7 +61,8 @@ public void SetUp()
             _addArtistService = Mocker.Resolve<AddArtistService>();
 
             Mocker.SetConstant<IRefreshTrackService>(Mocker.Resolve<RefreshTrackService>());
-            Mocker.SetConstant<IAddAlbumService>(Mocker.Resolve<AddAlbumService>());
+            Mocker.SetConstant<IRefreshAlbumReleaseService>(Mocker.Resolve<RefreshAlbumReleaseService>());
+            Mocker.SetConstant<IRefreshAlbumService>(Mocker.Resolve<RefreshAlbumService>());
             _refreshArtistService = Mocker.Resolve<RefreshArtistService>();
 
             Mocker.GetMock<IAddArtistValidator>().Setup(x => x.Validate(It.IsAny<Artist>())).Returns(new ValidationResult());

src/NzbDrone.Core.Test/MusicTests/RefreshArtistServiceFixture.cs

@@ -11,6 +11,9 @@
 using NzbDrone.Core.Music;
 using NzbDrone.Core.Music.Commands;
 using NzbDrone.Test.Common;
+using NzbDrone.Core.MediaFiles;
+using NzbDrone.Core.History;
+using NzbDrone.Core.Music.Events;
 
 namespace NzbDrone.Core.Test.MusicTests
 {
@@ -41,17 +44,24 @@ public void Setup()
                 .With(a => a.Metadata = metadata)
                 .Build();
 
-            Mocker.GetMock<IArtistService>()
+            Mocker.GetMock<IArtistService>(MockBehavior.Strict)
                   .Setup(s => s.GetArtist(_artist.Id))
                   .Returns(_artist);
 
-            Mocker.GetMock<IAlbumService>()
-                .Setup(s => s.GetAlbumsForRefresh(It.IsAny<int>(), It.IsAny<IEnumerable<string>>()))
-                .Returns(new List<Album>());
+            Mocker.GetMock<IAlbumService>(MockBehavior.Strict)
+                .Setup(s => s.InsertMany(It.IsAny<List<Album>>()));
 
             Mocker.GetMock<IProvideArtistInfo>()
                   .Setup(s => s.GetArtistInfo(It.IsAny<string>(), It.IsAny<int>()))
                   .Callback(() => { throw new ArtistNotFoundException(_artist.ForeignArtistId); });
+
+            Mocker.GetMock<IMediaFileService>()
+                .Setup(x => x.GetFilesByArtist(It.IsAny<int>()))
+                .Returns(new List<TrackFile>());
+
+            Mocker.GetMock<IHistoryService>()
+                .Setup(x => x.GetByArtist(It.IsAny<int>(), It.IsAny<HistoryEventType?>()))
+                .Returns(new List<History.History>());
         }
 
         private void GivenNewArtistInfo(Artist artist)
@@ -60,81 +70,207 @@ private void GivenNewArtistInfo(Artist artist)
                   .Setup(s => s.GetArtistInfo(_artist.ForeignArtistId, _artist.MetadataProfileId))
                   .Returns(artist);
         }
+
+        private void GivenArtistFiles()
+        {
+            Mocker.GetMock<IMediaFileService>()
+                  .Setup(x => x.GetFilesByArtist(It.IsAny<int>()))
+                  .Returns(Builder<TrackFile>.CreateListOfSize(1).BuildList());
+        }
+
+        private void GivenAlbumsForRefresh()
+        {
+            Mocker.GetMock<IAlbumService>(MockBehavior.Strict)
+                .Setup(s => s.GetAlbumsForRefresh(It.IsAny<int>(), It.IsAny<IEnumerable<string>>()))
+                .Returns(new List<Album>());
+        }
+
+        private void AllowArtistUpdate()
+        {
+            Mocker.GetMock<IArtistService>(MockBehavior.Strict)
+                .Setup(x => x.UpdateArtist(It.IsAny<Artist>()))
+                .Returns((Artist a) => a);
+        }
 
         [Test]
-        public void should_log_error_if_musicbrainz_id_not_found()
+        public void should_not_publish_artist_updated_event_if_metadata_not_updated()
         {
-            Subject.Execute(new RefreshArtistCommand(_artist.Id));
+            var newArtistInfo = _artist.JsonClone();
+            newArtistInfo.Metadata = _artist.Metadata.Value.JsonClone();
+            newArtistInfo.Albums = _albums;
 
-            Mocker.GetMock<IArtistService>()
-                .Verify(v => v.UpdateArtist(It.IsAny<Artist>()), Times.Never());
+            GivenNewArtistInfo(newArtistInfo);
+            GivenAlbumsForRefresh();
+            AllowArtistUpdate();
 
-            ExceptionVerification.ExpectedErrors(1);
+            Subject.Execute(new RefreshArtistCommand(_artist.Id));
+
+            VerifyEventNotPublished<ArtistUpdatedEvent>();
         }
 
         [Test]
-        public void should_update_if_musicbrainz_id_changed()
+        public void should_publish_artist_updated_event_if_metadata_updated()
         {
             var newArtistInfo = _artist.JsonClone();
             newArtistInfo.Metadata = _artist.Metadata.Value.JsonClone();
+            newArtistInfo.Metadata.Value.Images = new List<MediaCover.MediaCover> {
+                new MediaCover.MediaCover(MediaCover.MediaCoverTypes.Logo, "dummy")
+            };
             newArtistInfo.Albums = _albums;
-            newArtistInfo.ForeignArtistId = _artist.ForeignArtistId + 1;
 
             GivenNewArtistInfo(newArtistInfo);
+            GivenAlbumsForRefresh();
+            AllowArtistUpdate();
+
+            Subject.Execute(new RefreshArtistCommand(_artist.Id));
+
+            VerifyEventPublished<ArtistUpdatedEvent>();
+        }
+
+        [Test]
+        public void should_log_error_and_delete_if_musicbrainz_id_not_found_and_artist_has_no_files()
+        {
+            Mocker.GetMock<IArtistService>()
+                .Setup(x => x.DeleteArtist(It.IsAny<int>(), It.IsAny<bool>(), It.IsAny<bool>()));
 
             Subject.Execute(new RefreshArtistCommand(_artist.Id));
 
             Mocker.GetMock<IArtistService>()
-                .Verify(v => v.UpdateArtist(It.Is<Artist>(s => s.ForeignArtistId == newArtistInfo.ForeignArtistId)));
+                .Verify(v => v.UpdateArtist(It.IsAny<Artist>()), Times.Never());
+
+            Mocker.GetMock<IArtistService>()
+                .Verify(v => v.DeleteArtist(It.IsAny<int>(), It.IsAny<bool>(), It.IsAny<bool>()), Times.Once());
 
+            ExceptionVerification.ExpectedErrors(1);
             ExceptionVerification.ExpectedWarns(1);
         }
+
+        [Test]
+        public void should_log_error_but_not_delete_if_musicbrainz_id_not_found_and_artist_has_files()
+        {
+            GivenArtistFiles();
+            GivenAlbumsForRefresh();
+
+            Subject.Execute(new RefreshArtistCommand(_artist.Id));
+
+            Mocker.GetMock<IArtistService>()
+                .Verify(v => v.UpdateArtist(It.IsAny<Artist>()), Times.Never());
+
+            Mocker.GetMock<IArtistService>()
+                .Verify(v => v.DeleteArtist(It.IsAny<int>(), It.IsAny<bool>(), It.IsAny<bool>()), Times.Never());
+
+            ExceptionVerification.ExpectedErrors(2);
+        }
 
         [Test]
-        [Ignore("This test needs to be re-written as we no longer store albums in artist table or object")]
-        public void should_not_throw_if_duplicate_album_is_in_existing_info()
+        public void should_update_if_musicbrainz_id_changed_and_no_clash()
         {
             var newArtistInfo = _artist.JsonClone();
-            newArtistInfo.Albums.Value.Add(Builder<Album>.CreateNew()
-                                                  .With(s => s.ForeignAlbumId = "2")
-                                                  .Build());
+            newArtistInfo.Metadata = _artist.Metadata.Value.JsonClone();
+            newArtistInfo.Albums = _albums;
+            newArtistInfo.ForeignArtistId = _artist.ForeignArtistId + 1;
+            newArtistInfo.Metadata.Value.Id = 100;
+
+            GivenNewArtistInfo(newArtistInfo);
 
-            _artist.Albums.Value.Add(Builder<Album>.CreateNew()
-                                            .With(s => s.ForeignAlbumId = "2")
-                                            .Build());
+            var seq = new MockSequence();
 
-            _artist.Albums.Value.Add(Builder<Album>.CreateNew()
-                                            .With(s => s.ForeignAlbumId = "2")
-                                            .Build());
+            Mocker.GetMock<IArtistService>(MockBehavior.Strict)
+                .Setup(x => x.FindById(newArtistInfo.ForeignArtistId))
+                .Returns(default(Artist));
 
-            GivenNewArtistInfo(newArtistInfo);
+            // Make sure that the artist is updated before we refresh the albums
+            Mocker.GetMock<IArtistService>(MockBehavior.Strict)
+                .InSequence(seq)
+                .Setup(x => x.UpdateArtist(It.IsAny<Artist>()))
+                .Returns((Artist a) => a);
+
+            Mocker.GetMock<IAlbumService>(MockBehavior.Strict)
+                .InSequence(seq)
+                .Setup(x => x.GetAlbumsForRefresh(It.IsAny<int>(), It.IsAny<IEnumerable<string>>()))
+                .Returns(new List<Album>());
+
+            // Update called twice for a move/merge
+            Mocker.GetMock<IArtistService>(MockBehavior.Strict)
+                .InSequence(seq)
+                .Setup(x => x.UpdateArtist(It.IsAny<Artist>()))
+                .Returns((Artist a) => a);
 
             Subject.Execute(new RefreshArtistCommand(_artist.Id));
 
             Mocker.GetMock<IArtistService>()
-                  .Verify(v => v.UpdateArtist(It.Is<Artist>(s => s.Albums.Value.Count == 2)));
+                .Verify(v => v.UpdateArtist(It.Is<Artist>(s => s.ArtistMetadataId == 100 && s.ForeignArtistId == newArtistInfo.ForeignArtistId)),
+                        Times.Exactly(2));
         }
 
         [Test]
-        [Ignore("This test needs to be re-written as we no longer store albums in artist table or object")]
-        public void should_filter_duplicate_albums()
+        public void should_merge_if_musicbrainz_id_changed_and_new_id_already_exists()
         {
-            var newArtistInfo = _artist.JsonClone();
-            newArtistInfo.Albums.Value.Add(Builder<Album>.CreateNew()
-                                                  .With(s => s.ForeignAlbumId = "2")
-                                                  .Build());
+            var existing = _artist;
+
+            var clash = _artist.JsonClone();
+            clash.Id = 100;
+            clash.Metadata = existing.Metadata.Value.JsonClone();
+            clash.Metadata.Value.Id = 101;
+            clash.Metadata.Value.ForeignArtistId = clash.Metadata.Value.ForeignArtistId + 1;
 
-            newArtistInfo.Albums.Value.Add(Builder<Album>.CreateNew()
-                                                  .With(s => s.ForeignAlbumId = "2")
-                                                  .Build());
+            Mocker.GetMock<IArtistService>(MockBehavior.Strict)
+                .Setup(x => x.FindById(clash.Metadata.Value.ForeignArtistId))
+                .Returns(clash);
+
+            var newArtistInfo = clash.JsonClone();
+            newArtistInfo.Metadata = clash.Metadata.Value.JsonClone();
+            newArtistInfo.Albums = _albums.JsonClone();
+            newArtistInfo.Albums.Value.ForEach(x => x.Id = 0);
 
             GivenNewArtistInfo(newArtistInfo);
 
+            var seq = new MockSequence();
+
+            // Make sure that the artist is updated before we refresh the albums
+            Mocker.GetMock<IAlbumService>(MockBehavior.Strict)
+                .InSequence(seq)
+                .Setup(x => x.GetAlbumsByArtist(existing.Id))
+                .Returns(_albums);
+
+            Mocker.GetMock<IAlbumService>(MockBehavior.Strict)
+                .InSequence(seq)
+                .Setup(x => x.UpdateMany(It.IsAny<List<Album>>()));
+
+            Mocker.GetMock<IArtistService>(MockBehavior.Strict)
+                .InSequence(seq)
+                .Setup(x => x.DeleteArtist(existing.Id, It.IsAny<bool>(), false));
+
+            Mocker.GetMock<IArtistService>(MockBehavior.Strict)
+                .InSequence(seq)
+                .Setup(x => x.UpdateArtist(It.Is<Artist>(a => a.Id == clash.Id)))
+                .Returns((Artist a) => a);
+
+            Mocker.GetMock<IAlbumService>(MockBehavior.Strict)
+                .InSequence(seq)
+                .Setup(x => x.GetAlbumsForRefresh(clash.ArtistMetadataId, It.IsAny<IEnumerable<string>>()))
+                .Returns(_albums);
+
+            // Update called twice for a move/merge
+            Mocker.GetMock<IArtistService>(MockBehavior.Strict)
+                .InSequence(seq)
+                .Setup(x => x.UpdateArtist(It.IsAny<Artist>()))
+                .Returns((Artist a) => a);
+
             Subject.Execute(new RefreshArtistCommand(_artist.Id));
 
+            // the retained artist gets updated
+            Mocker.GetMock<IArtistService>()
+                .Verify(v => v.UpdateArtist(It.Is<Artist>(s => s.Id == clash.Id)), Times.Exactly(2));
+
+            // the old one gets removed
             Mocker.GetMock<IArtistService>()
-                  .Verify(v => v.UpdateArtist(It.Is<Artist>(s => s.Albums.Value.Count == 2)));
+                .Verify(v => v.DeleteArtist(existing.Id, false, false));
+
+            Mocker.GetMock<IAlbumService>()
+                .Verify(v => v.UpdateMany(It.Is<List<Album>>(x => x.Count == _albums.Count)));
 
+            ExceptionVerification.ExpectedWarns(1);
         }
     }
 }

src/NzbDrone.Core.Test/NzbDrone.Core.Test.csproj

@@ -275,7 +275,6 @@
     <Compile Include="MetadataSource\SkyHook\SkyHookProxySearchFixture.cs" />
     <Compile Include="MetadataSource\SearchArtistComparerFixture.cs" />
     <Compile Include="MetadataSource\SkyHook\SkyHookProxyFixture.cs" />
-    <Compile Include="MusicTests\AddAlbumFixture.cs" />
     <Compile Include="MusicTests\AlbumServiceFixture.cs" />
     <Compile Include="MusicTests\AddArtistFixture.cs" />
     <Compile Include="MusicTests\AlbumMonitoredServiceTests\AlbumMonitoredServiceFixture.cs" />

src/NzbDrone.Core/History/HistoryService.cs

@@ -28,6 +28,7 @@ public interface IHistoryService
         List<History> Find(string downloadId, HistoryEventType eventType);
         List<History> FindByDownloadId(string downloadId);
         List<History> Since(DateTime date, HistoryEventType? eventType);
+        void UpdateMany(IList<History> items);
     }
 
     public class HistoryService : IHistoryService,
@@ -381,5 +382,10 @@ public List<History> Since(DateTime date, HistoryEventType? eventType)
         {
             return _historyRepository.Since(date, eventType);
         }
+
+        public void UpdateMany(IList<History> items)
+        {
+            _historyRepository.UpdateMany(items);
+        }
     }
 }

src/NzbDrone.Core/MetadataSource/SkyHook/Resource/ArtistResource.cs

@@ -1,14 +1,13 @@
-using System;
 using System.Collections.Generic;
-using System.Linq;
-using System.Text;
 
 namespace NzbDrone.Core.MetadataSource.SkyHook.Resource
 {
     public class ArtistResource
     {
-        public ArtistResource() {
+        public ArtistResource()
+        {
             Albums = new List<AlbumResource>();
+            Genres = new List<string>();
         }
 
         public List<string> Genres { get; set; }

src/NzbDrone.Core/MetadataSource/SkyHook/SkyHookProxy.cs

@@ -82,10 +82,7 @@ public Artist GetArtistInfo(string foreignArtistId, int metadataProfileId)
             artist.SortName = Parser.Parser.NormalizeTitle(artist.Metadata.Value.Name);
 
             artist.Albums = FilterAlbums(httpResponse.Resource.Albums, metadataProfileId)
-                .Select(x => new Album {
-                        ForeignAlbumId = x.Id
-                    })
-                .ToList();
+                .Select(x => MapAlbum(x, null)).ToList();
 
             return artist;
         }
@@ -136,6 +133,7 @@ public Tuple<string, Album, List<ArtistMetadata>> GetAlbumInfo(string foreignAlb
             var artists = httpResponse.Resource.Artists.Select(MapArtistMetadata).ToList();
             var artistDict = artists.ToDictionary(x => x.ForeignArtistId, x => x);
             var album = MapAlbum(httpResponse.Resource, artistDict);
+            album.ArtistMetadata = artistDict[httpResponse.Resource.ArtistId];
 
             return new Tuple<string, Album, List<ArtistMetadata>>(httpResponse.Resource.ArtistId, album, artists);
         }
@@ -181,8 +179,6 @@ public List<Artist> SearchForNewArtist(string title)
                                     .SetSegment("route", "search")
                                     .AddQueryParam("type", "artist")
                                     .AddQueryParam("query", title.ToLower().Trim())
-                                    //.AddQueryParam("images","false") // Should pass these on import search to avoid looking to fanart and wiki 
-                                    //.AddQueryParam("overview","false")
                                     .Build();