Complete unit tests for artist

src/core/collection/artist.rs

@@ -56,6 +56,64 @@ impl Artist {
             albums: vec![],
         }
     }
+
+    fn merge_album_with_lib_id(
+        primary_albums: &mut Vec<Album>,
+        mut secondary_album: Album,
+    ) -> Option<Album> {
+        if let lib_id @ AlbumLibId::Some(_) | lib_id @ AlbumLibId::Singleton =
+            secondary_album.meta.id.lib_id
+        {
+            if let Some(ref mut primary_album) = primary_albums
+                .iter_mut()
+                .find(|album| album.meta.id.lib_id == lib_id)
+            {
+                primary_album.merge_in_place(secondary_album);
+                return None;
+            }
+
+            secondary_album.meta.id.lib_id = AlbumLibId::None;
+        }
+
+        Some(secondary_album)
+    }
+
+    fn merge_albums_with_lib_id(
+        primary_albums: &mut Vec<Album>,
+        mut secondary_albums: Vec<Album>,
+    ) -> HashMap<String, Vec<Album>> {
+        let mut secondary_without_id = HashMap::<String, Vec<Album>>::new();
+        for secondary_album in secondary_albums.drain(..) {
+            if let Some(secondary_album) =
+                Artist::merge_album_with_lib_id(primary_albums, secondary_album)
+            {
+                secondary_without_id
+                    .entry(secondary_album.meta.id.title.clone())
+                    .or_default()
+                    .push(secondary_album);
+            }
+        }
+        secondary_without_id
+    }
+
+    fn merge_albums_with_title(
+        primary_albums: &mut Vec<Album>,
+        mut secondary_without_id: HashMap<String, Vec<Album>>,
+    ) {
+        for (title, mut secondary_albums) in secondary_without_id.drain() {
+            match primary_albums
+                .iter_mut()
+                .find(|album| album.meta.id.title == title)
+            {
+                Some(ref mut primary_album) => {
+                    // We do not support merging multiple DB albums with same title yet.
+                    assert_eq!(secondary_albums.len(), 1);
+                    primary_album.merge_in_place(secondary_albums.pop().unwrap())
+                }
+                None => primary_albums.append(&mut secondary_albums),
+            }
+        }
+    }
 }
 
 impl PartialOrd for Artist {
@@ -75,45 +133,11 @@ impl Merge for Artist {
         self.meta.merge_in_place(other.meta);
 
         let mut primary_albums = mem::take(&mut self.albums);
-        let mut secondary_albums = other.albums;
+        let secondary_albums = other.albums;
 
-        let mut secondary_without_id = HashMap::<String, Vec<Album>>::new();
+        let secondary_without_id =
-        for mut secondary_album in secondary_albums.drain(..) {
+            Artist::merge_albums_with_lib_id(&mut primary_albums, secondary_albums);
-            match secondary_album.meta.id.lib_id {
+        Artist::merge_albums_with_title(&mut primary_albums, secondary_without_id);
-                lib_id @ AlbumLibId::Some(_) | lib_id @ AlbumLibId::Singleton => {
-                    match primary_albums
-                        .iter_mut()
-                        .find(|album| album.meta.id.lib_id == lib_id)
-                    {
-                        Some(ref mut primary_album) => {
-                            primary_album.merge_in_place(secondary_album)
-                        }
-                        None => {
-                            secondary_album.meta.id.lib_id = AlbumLibId::None;
-                            primary_albums.push(secondary_album);
-                        }
-                    }
-                }
-                AlbumLibId::None => secondary_without_id
-                    .entry(secondary_album.meta.id.title.clone())
-                    .or_default()
-                    .push(secondary_album),
-            }
-        }
-
-        for (title, mut secondary_albums) in secondary_without_id.drain() {
-            match primary_albums
-                .iter_mut()
-                .find(|album| album.meta.id.title == title)
-            {
-                Some(ref mut primary_album) => {
-                    // We do not support merging multiple DB albums with same title yet.
-                    assert_eq!(secondary_albums.len(), 1);
-                    primary_album.merge_in_place(secondary_albums.pop().unwrap())
-                }
-                None => primary_albums.append(&mut secondary_albums),
-            }
-        }
 
         primary_albums.sort_unstable();
         self.albums = primary_albums;
@@ -454,6 +478,12 @@ mod tests {
         assert!(info.properties.is_empty());
     }
 
+    fn reset_lib_id(albums: &mut [Album]) {
+        for album in albums.iter_mut() {
+            album.meta.id.lib_id = AlbumLibId::None;
+        }
+    }
+
     #[test]
     fn merge_artist_no_overlap() {
         let left = FULL_COLLECTION[0].to_owned();
@@ -468,13 +498,25 @@ mod tests {
             .info
             .properties
             .merge(right.clone().meta.info.properties);
-        expected.albums.append(&mut right.albums.clone());
+
+        // If an album in the secondary (right) collection has a lib id but there is no match in the
+        // primary (left) collection then it is removed.
+        let mut right_albums_without_lib_id = right.albums.clone();
+        reset_lib_id(&mut right_albums_without_lib_id);
+        expected.albums.extend(right_albums_without_lib_id);
         expected.albums.sort_unstable();
 
         let merged = left.clone().merge(right.clone());
         assert_eq!(expected, merged);
 
-        // Non-overlapping merge should be commutative.
+        // Non-overlapping merge should be commutative. Except for lib id which will be erased from
+        // the albums in the secondary collection.
+        expected.albums = right.albums.clone();
+        let mut left_albums_without_lib_id = left.albums.clone();
+        reset_lib_id(&mut left_albums_without_lib_id);
+        expected.albums.extend(left_albums_without_lib_id);
+        expected.albums.sort_unstable();
+
         let merged = right.clone().merge(left.clone());
         assert_eq!(expected, merged);
     }
@@ -487,6 +529,9 @@ mod tests {
         left.albums.push(right.albums[0].clone());
         left.albums.sort_unstable();
 
+        // Albums on right without a match on the left will lose their lib id.
+        reset_lib_id(&mut right.albums[1..]);
+
         let mut expected = left.clone();
         expected.meta.info.properties = expected
             .meta