diff --git a/openlp/plugins/songs/lib/mediaitem.py b/openlp/plugins/songs/lib/mediaitem.py index bee0e0f0d..d57c8fbcc 100644 --- a/openlp/plugins/songs/lib/mediaitem.py +++ b/openlp/plugins/songs/lib/mediaitem.py @@ -527,15 +527,7 @@ class SongMediaItem(MediaManagerItem): add_song = True if search_results: for song in search_results: - author_list = item.data_string['authors'] - same_authors = True - for author in song.authors: - if author.display_name in author_list: - author_list = author_list.replace(author.display_name, '', 1) - else: - same_authors = False - break - if same_authors and author_list.strip(', ') == '': + if self._authors_match(song, item.data_string['authors']): add_song = False edit_id = song.id break @@ -561,6 +553,23 @@ class SongMediaItem(MediaManagerItem): self.generate_footer(item, song) return item + def _authors_match(self, song, authors): + """ + Checks whether authors from a song in the database match the authors of the song to be imported. + + :param song: A list of authors from the song in the database + :param authors: A string with authors from the song to be imported + :return: True when Authors do match, else False. + """ + author_list = authors.split(', ') + for author in song.authors: + if author.display_name in author_list: + author_list.remove(author.display_name) + else: + return False + # List must be empty at the end + return not author_list + def search(self, string, show_error): """ Search for some songs diff --git a/tests/functional/openlp_plugins/songs/test_mediaitem.py b/tests/functional/openlp_plugins/songs/test_mediaitem.py index 22291c6a6..bc22a4577 100644 --- a/tests/functional/openlp_plugins/songs/test_mediaitem.py +++ b/tests/functional/openlp_plugins/songs/test_mediaitem.py @@ -153,3 +153,52 @@ class TestMediaItem(TestCase, TestMixin): # THEN: The songbook should be in the footer self.assertEqual(service_item.raw_footer, ['My Song', 'My copyright', 'My songbook #12']) + + def authors_match_test(self): + """ + Test the author matching when importing a song from a service + """ + # GIVEN: A song and a string with authors + song = MagicMock() + song.authors = [] + author = MagicMock() + author.display_name = "Hans Wurst" + song.authors.append(author) + author2 = MagicMock() + author2.display_name = "Max Mustermann" + song.authors.append(author2) + # There are occasions where an author appears twice in a song (with different types). + # We need to make sure that this case works (lp#1313538) + author3 = MagicMock() + author3.display_name = "Max Mustermann" + song.authors.append(author3) + authors_str = "Hans Wurst, Max Mustermann, Max Mustermann" + + # WHEN: Checking for matching + result = self.media_item._authors_match(song, authors_str) + + # THEN: They should match + self.assertTrue(result, "Authors should match") + + def authors_dont_match_test(self): + # GIVEN: A song and a string with authors + song = MagicMock() + song.authors = [] + author = MagicMock() + author.display_name = "Hans Wurst" + song.authors.append(author) + author2 = MagicMock() + author2.display_name = "Max Mustermann" + song.authors.append(author2) + # There are occasions where an author appears twice in a song (with different types). + # We need to make sure that this case works (lp#1313538) + author3 = MagicMock() + author3.display_name = "Max Mustermann" + song.authors.append(author3) + + # WHEN: An author is missing in the string + authors_str = "Hans Wurst, Max Mustermann" + result = self.media_item._authors_match(song, authors_str) + + # THEN: They should not match + self.assertFalse(result, "Authors should not match")