When matching an MB entity to an owned album check if it isn't already in the collection (#250)

Closes #249

Reviewed-on: #250

src/tui/app/machine/match_state.rs

@@ -6,10 +6,13 @@ use musichoard::collection::{
     musicbrainz::{MbRefOption, Mbid},
 };
 
-use crate::tui::app::{
+use crate::tui::{
-    machine::{fetch_state::FetchState, input::Input, App, AppInner, AppMachine},
+    app::{
-    AlbumMatches, AppPublicState, AppState, ArtistMatches, Delta, EntityMatches, IAppInteractMatch,
+        machine::{fetch_state::FetchState, input::Input, App, AppInner, AppMachine},
-    MatchOption, MatchStatePublic, WidgetState,
+        AlbumMatches, AppPublicState, AppState, ArtistMatches, Delta, EntityMatches,
+        IAppInteractMatch, MatchOption, MatchStatePublic, WidgetState,
+    },
+    lib::IMusicHoard,
 };
 
 struct ArtistInfoTuple {
@@ -200,6 +203,39 @@ impl AppMachine<MatchState> {
         self.input.replace(Input::default());
         self.into()
     }
+
+    fn select_artist(
+        mh: &mut Box<dyn IMusicHoard>,
+        matches: &ArtistMatches,
+        tuple: ArtistInfoTuple,
+    ) -> Result<(), musichoard::Error> {
+        mh.merge_artist_info(&matches.matching.id, tuple.info)?;
+        mh.set_artist_mb_ref(&matches.matching.id, tuple.mb_ref)
+    }
+
+    fn select_album(
+        mh: &mut Box<dyn IMusicHoard>,
+        matches: &AlbumMatches,
+        tuple: AlbumInfoTuple,
+    ) -> Result<(), musichoard::Error> {
+        let coll = mh.get_collection();
+        let mut clashing = vec![];
+        if let Some(artist) = coll.iter().find(|artist| artist.meta.id == matches.artist) {
+            // While we expect only one, there is nothing stopping anybody from having multiple
+            // different albums with the same MBID.
+            let iter = artist.albums.iter();
+            let filtered = iter
+                .filter(|album| album.meta.id != matches.matching)
+                .filter(|album| album.meta.id.mb_ref == tuple.mb_ref);
+            clashing.extend(filtered.map(|a| &a.meta.id).cloned())
+        }
+
+        for album in clashing.into_iter() {
+            mh.remove_album(&matches.artist, &album)?;
+        }
+        mh.merge_album_info(&matches.artist, &matches.matching, tuple.info)?;
+        mh.set_album_mb_ref(&matches.artist, &matches.matching, tuple.mb_ref)
+    }
 }
 
 impl From<AppMachine<MatchState>> for App {
@@ -246,17 +282,11 @@ impl IAppInteractMatch for AppMachine<MatchState> {
         let mh = &mut self.inner.music_hoard;
         let result = match self.state.current {
             EntityMatches::Artist(ref mut matches) => match matches.list.extract_info(index) {
-                InfoOption::Info(tuple) => mh
+                InfoOption::Info(tuple) => Self::select_artist(mh, matches, tuple),
-                    .merge_artist_info(&matches.matching.id, tuple.info)
-                    .and_then(|()| mh.set_artist_mb_ref(&matches.matching.id, tuple.mb_ref)),
                 InfoOption::NeedInput => return self.get_input(),
             },
             EntityMatches::Album(ref mut matches) => match matches.list.extract_info(index) {
-                InfoOption::Info(tuple) => mh
+                InfoOption::Info(tuple) => Self::select_album(mh, matches, tuple),
-                    .merge_album_info(&matches.artist, &matches.matching, tuple.info)
-                    .and_then(|()| {
-                        mh.set_album_mb_ref(&matches.artist, &matches.matching, tuple.mb_ref)
-                    }),
                 InfoOption::NeedInput => return self.get_input(),
             },
         };
@@ -282,8 +312,10 @@ mod tests {
         Sequence,
     };
     use musichoard::collection::{
-        album::{AlbumDate, AlbumId, AlbumInfo, AlbumMeta, AlbumPrimaryType, AlbumSecondaryType},
+        album::{
-        artist::{ArtistId, ArtistMeta},
+            Album, AlbumDate, AlbumId, AlbumInfo, AlbumMeta, AlbumPrimaryType, AlbumSecondaryType,
+        },
+        artist::{Artist, ArtistId, ArtistMeta},
     };
 
     use crate::tui::{
@@ -593,6 +625,58 @@ mod tests {
         matches.select().unwrap_fetch();
     }
 
+    #[test]
+    fn set_album_info_with_clashing_mbid() {
+        let matches_info = album_match();
+
+        // The collection below should trigger MH to remove the album with the same MBID but not
+        // matching album_id.
+
+        // (1) Same artist as matches_info.
+        let mut artist = Artist::new(ArtistId::new("Artist"));
+
+        // (2) An album with the same album_id as the selected one.
+        artist.albums.push(Album::new(AlbumId::new("Album")));
+
+        // (3) An album with a different album_id than the selected one.
+        // (4) This album has an MBID that is identical to that of the selected match.
+        let mb_ref = AlbumMbRef::Some(mbid().into());
+        let removed = Album::new(AlbumId::new("Not the Same").with_mb_ref(mb_ref));
+        artist.albums.push(removed.clone());
+
+        let (_tx, rx) = mpsc::channel();
+        let app_matches = MatchState::new(matches_info.clone(), FetchState::search(rx));
+
+        let mut music_hoard = music_hoard(vec![artist]);
+        match matches_info {
+            EntityMatches::Artist(_) => panic!(),
+            EntityMatches::Album(matches) => {
+                let artist = matches.artist.clone();
+
+                let mut seq = Sequence::new();
+                music_hoard
+                    .expect_remove_album()
+                    .times(1)
+                    .with(eq(artist), eq(removed.meta.id.clone()))
+                    .in_sequence(&mut seq)
+                    .return_once(|_, _| Ok(()));
+                music_hoard
+                    .expect_merge_album_info()
+                    .times(1)
+                    .in_sequence(&mut seq)
+                    .return_once(|_, _, _| Ok(()));
+                music_hoard
+                    .expect_set_album_mb_ref()
+                    .times(1)
+                    .in_sequence(&mut seq)
+                    .return_once(|_, _, _| Ok(()));
+            }
+        }
+
+        let matches = AppMachine::match_state(inner(music_hoard), app_matches);
+        matches.select().unwrap_fetch();
+    }
+
     #[test]
     fn set_info_error() {
         let matches_info = artist_match();

src/tui/lib/mod.rs

@@ -25,6 +25,11 @@ pub trait IMusicHoard {
         artist_id: &ArtistId,
         album_meta: AlbumMeta,
     ) -> Result<(), musichoard::Error>;
+    fn remove_album(
+        &mut self,
+        artist_id: &ArtistId,
+        album_id: &AlbumId,
+    ) -> Result<(), musichoard::Error>;
 
     fn set_artist_mb_ref(
         &mut self,
@@ -72,6 +77,14 @@ impl<Database: IDatabase, Library: ILibrary> IMusicHoard for MusicHoard<Database
         <Self as IMusicHoardDatabase>::add_album(self, artist_id, album_meta)
     }
 
+    fn remove_album(
+        &mut self,
+        artist_id: &ArtistId,
+        album_id: &AlbumId,
+    ) -> Result<(), musichoard::Error> {
+        <Self as IMusicHoardDatabase>::remove_album(self, artist_id, album_id)
+    }
+
     fn set_artist_mb_ref(
         &mut self,
         artist_id: &ArtistId,