Merge lp:~raoul-snyman/openlp/bug-1629079-2.4 into lp:openlp/2.4
- bug-1629079-2.4
- Merge into 2.4
Proposed by
Raoul Snyman
Status: | Merged |
---|---|
Merged at revision: | 2656 |
Proposed branch: | lp:~raoul-snyman/openlp/bug-1629079-2.4 |
Merge into: | lp:openlp/2.4 |
Diff against target: |
301 lines (+155/-17) 2 files modified
openlp/plugins/songs/lib/songselect.py (+3/-3) tests/functional/openlp_plugins/songs/test_songselect.py (+152/-14) |
To merge this branch: | bzr merge lp:~raoul-snyman/openlp/bug-1629079-2.4 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Tim Bentley | Approve | ||
Tomas Groth | Approve | ||
Review via email: mp+307380@code.launchpad.net |
Commit message
Description of the change
Fix bug #1629079: Attribute error when importing from SongSelect
Add this to your merge proposal:
-------
lp:~raoul-snyman/openlp/bug-1629079-2.4 (revision 2657)
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
To post a comment you must log in.
Revision history for this message
Tomas Groth (tomasgroth) : | # |
review:
Approve
Revision history for this message
Tim Bentley (trb143) : | # |
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'openlp/plugins/songs/lib/songselect.py' |
2 | --- openlp/plugins/songs/lib/songselect.py 2016-08-12 11:52:35 +0000 |
3 | +++ openlp/plugins/songs/lib/songselect.py 2016-10-01 19:29:22 +0000 |
4 | @@ -23,7 +23,6 @@ |
5 | The :mod:`~openlp.plugins.songs.lib.songselect` module contains the SongSelect importer itself. |
6 | """ |
7 | import logging |
8 | -import sys |
9 | import random |
10 | import re |
11 | from http.cookiejar import CookieJar |
12 | @@ -114,7 +113,7 @@ |
13 | try: |
14 | self.opener.open(LOGOUT_URL) |
15 | except (TypeError, URLError) as error: |
16 | - log.exception('Could not log of SongSelect, %s', error) |
17 | + log.exception('Could not log out of SongSelect, %s', error) |
18 | |
19 | def search(self, search_text, max_results, callback=None): |
20 | """ |
21 | @@ -251,11 +250,12 @@ |
22 | last_name = name_parts[1] |
23 | author = Author.populate(first_name=first_name, last_name=last_name, display_name=author_name) |
24 | db_song.add_author(author) |
25 | + db_song.topics = [] |
26 | for topic_name in song.get('topics', []): |
27 | topic = self.db_manager.get_object_filtered(Topic, Topic.name == topic_name) |
28 | if not topic: |
29 | topic = Topic.populate(name=topic_name) |
30 | - db_song.add_topic(topic) |
31 | + db_song.topics.append(topic) |
32 | self.db_manager.save_object(db_song) |
33 | return db_song |
34 | |
35 | |
36 | === modified file 'tests/functional/openlp_plugins/songs/test_songselect.py' |
37 | --- tests/functional/openlp_plugins/songs/test_songselect.py 2016-08-11 21:13:12 +0000 |
38 | +++ tests/functional/openlp_plugins/songs/test_songselect.py 2016-10-01 19:29:22 +0000 |
39 | @@ -26,16 +26,16 @@ |
40 | from unittest import TestCase |
41 | from urllib.error import URLError |
42 | |
43 | +from bs4 import NavigableString |
44 | from PyQt5 import QtWidgets |
45 | |
46 | -from tests.helpers.songfileimport import SongImportTestHelper |
47 | from openlp.core import Registry |
48 | from openlp.plugins.songs.forms.songselectform import SongSelectForm, SearchWorker |
49 | from openlp.plugins.songs.lib import Song |
50 | from openlp.plugins.songs.lib.songselect import SongSelectImport, LOGOUT_URL, BASE_URL |
51 | -from openlp.plugins.songs.lib.importers.cclifile import CCLIFileImport |
52 | |
53 | from tests.functional import MagicMock, patch, call |
54 | +from tests.helpers.songfileimport import SongImportTestHelper |
55 | from tests.helpers.testmixin import TestMixin |
56 | |
57 | TEST_PATH = os.path.abspath( |
58 | @@ -63,6 +63,30 @@ |
59 | |
60 | @patch('openlp.plugins.songs.lib.songselect.build_opener') |
61 | @patch('openlp.plugins.songs.lib.songselect.BeautifulSoup') |
62 | + def login_fails_type_error_test(self, MockedBeautifulSoup, mocked_build_opener): |
63 | + """ |
64 | + Test that when logging in to SongSelect fails due to a TypeError, the login method returns False |
65 | + """ |
66 | + # GIVEN: A bunch of mocked out stuff and an importer object |
67 | + mocked_opener = MagicMock() |
68 | + mocked_build_opener.return_value = mocked_opener |
69 | + mocked_login_page = MagicMock() |
70 | + mocked_login_page.find.side_effect = [{'value': 'blah'}, None] |
71 | + MockedBeautifulSoup.side_effect = [mocked_login_page, TypeError('wrong type')] |
72 | + mock_callback = MagicMock() |
73 | + importer = SongSelectImport(None) |
74 | + |
75 | + # WHEN: The login method is called after being rigged to fail |
76 | + result = importer.login('username', 'password', mock_callback) |
77 | + |
78 | + # THEN: callback was called 3 times, open was called twice, find was called twice, and False was returned |
79 | + self.assertEqual(2, mock_callback.call_count, 'callback should have been called 3 times') |
80 | + self.assertEqual(1, mocked_login_page.find.call_count, 'find should have been called twice') |
81 | + self.assertEqual(2, mocked_opener.open.call_count, 'opener should have been called twice') |
82 | + self.assertFalse(result, 'The login method should have returned False') |
83 | + |
84 | + @patch('openlp.plugins.songs.lib.songselect.build_opener') |
85 | + @patch('openlp.plugins.songs.lib.songselect.BeautifulSoup') |
86 | def login_fails_test(self, MockedBeautifulSoup, mocked_build_opener): |
87 | """ |
88 | Test that when logging in to SongSelect fails, the login method returns False |
89 | @@ -144,6 +168,27 @@ |
90 | mocked_opener.open.assert_called_with(LOGOUT_URL) |
91 | |
92 | @patch('openlp.plugins.songs.lib.songselect.build_opener') |
93 | + @patch('openlp.plugins.songs.lib.songselect.log') |
94 | + def logout_fails_test(self, mocked_log, mocked_build_opener): |
95 | + """ |
96 | + Test that when the logout method is called, it logs the user out of SongSelect |
97 | + """ |
98 | + # GIVEN: A bunch of mocked out stuff and an importer object |
99 | + type_error = TypeError('wrong type') |
100 | + mocked_opener = MagicMock() |
101 | + mocked_opener.open.side_effect = type_error |
102 | + mocked_build_opener.return_value = mocked_opener |
103 | + importer = SongSelectImport(None) |
104 | + |
105 | + # WHEN: The login method is called after being rigged to fail |
106 | + importer.logout() |
107 | + |
108 | + # THEN: The opener is called once with the logout url |
109 | + self.assertEqual(1, mocked_opener.open.call_count, 'opener should have been called once') |
110 | + mocked_opener.open.assert_called_with(LOGOUT_URL) |
111 | + mocked_log.exception.assert_called_once_with('Could not log out of SongSelect, %s', type_error) |
112 | + |
113 | + @patch('openlp.plugins.songs.lib.songselect.build_opener') |
114 | @patch('openlp.plugins.songs.lib.songselect.BeautifulSoup') |
115 | def search_returns_no_results_test(self, MockedBeautifulSoup, mocked_build_opener): |
116 | """ |
117 | @@ -170,6 +215,30 @@ |
118 | |
119 | @patch('openlp.plugins.songs.lib.songselect.build_opener') |
120 | @patch('openlp.plugins.songs.lib.songselect.BeautifulSoup') |
121 | + def search_returns_no_results_after_exception_test(self, MockedBeautifulSoup, mocked_build_opener): |
122 | + """ |
123 | + Test that when the search finds no results, it simply returns an empty list |
124 | + """ |
125 | + # GIVEN: A bunch of mocked out stuff and an importer object |
126 | + mocked_opener = MagicMock() |
127 | + mocked_build_opener.return_value = mocked_opener |
128 | + mocked_results_page = MagicMock() |
129 | + mocked_results_page.find_all.return_value = [] |
130 | + MockedBeautifulSoup.side_effect = TypeError('wrong type') |
131 | + mock_callback = MagicMock() |
132 | + importer = SongSelectImport(None) |
133 | + |
134 | + # WHEN: The login method is called after being rigged to fail |
135 | + results = importer.search('text', 1000, mock_callback) |
136 | + |
137 | + # THEN: callback was never called, open was called once, find_all was called once, an empty list returned |
138 | + self.assertEqual(0, mock_callback.call_count, 'callback should not have been called') |
139 | + self.assertEqual(1, mocked_opener.open.call_count, 'open should have been called once') |
140 | + self.assertEqual(0, mocked_results_page.find_all.call_count, 'find_all should not have been called') |
141 | + self.assertEqual([], results, 'The search method should have returned an empty list') |
142 | + |
143 | + @patch('openlp.plugins.songs.lib.songselect.build_opener') |
144 | + @patch('openlp.plugins.songs.lib.songselect.BeautifulSoup') |
145 | def search_returns_two_results_test(self, MockedBeautifulSoup, mocked_build_opener): |
146 | """ |
147 | Test that when the search finds 2 results, it simply returns a list with 2 results |
148 | @@ -322,22 +391,47 @@ |
149 | Test that the get_song() method returns the correct song details |
150 | """ |
151 | # GIVEN: A bunch of mocked out stuff and an importer object |
152 | + mocked_ul_copyright = MagicMock() |
153 | + mocked_ul_copyright.find.side_effect = [True, False] |
154 | + mocked_ul_copyright.find_all.return_value = [ |
155 | + 'Copyright:', |
156 | + MagicMock(string='Copyright 1'), |
157 | + MagicMock(string='Copyright 2') |
158 | + ] |
159 | + |
160 | + mocked_ul_themes = MagicMock() |
161 | + mocked_ul_themes.find.side_effect = [False, True] |
162 | + mocked_ul_themes.find_all.return_value = [ |
163 | + 'Themes:', |
164 | + MagicMock(string='Theme 1'), |
165 | + MagicMock(string='Theme 2') |
166 | + ] |
167 | + |
168 | + mocked_ccli = MagicMock(string='CCLI: 123456 ') |
169 | + mocked_find_strong = MagicMock(return_value=mocked_ccli) |
170 | + mocked_find_li = MagicMock(return_value=mocked_find_strong) |
171 | + mocked_find_ul = MagicMock(return_value=mocked_find_li) |
172 | + |
173 | mocked_song_page = MagicMock() |
174 | - mocked_copyright = MagicMock() |
175 | - mocked_copyright.find_all.return_value = [MagicMock(string='Copyright 1'), MagicMock(string='Copyright 2')] |
176 | - mocked_song_page.find.side_effect = [ |
177 | - mocked_copyright, |
178 | - MagicMock(find=MagicMock(string='CCLI: 123456')) |
179 | + mocked_song_page.find_all.return_value = [ |
180 | + mocked_ul_copyright, |
181 | + mocked_ul_themes |
182 | ] |
183 | + mocked_song_page.find.return_value = mocked_find_ul |
184 | + |
185 | mocked_lyrics_page = MagicMock() |
186 | mocked_find_all = MagicMock() |
187 | mocked_find_all.side_effect = [ |
188 | [ |
189 | MagicMock(contents='The Lord told Noah: there\'s gonna be a floody, floody'), |
190 | - MagicMock(contents='So, rise and shine, and give God the glory, glory'), |
191 | + MagicMock(contents=NavigableString('So, rise and shine, and give God the glory, glory')), |
192 | MagicMock(contents='The Lord told Noah to build him an arky, arky') |
193 | ], |
194 | - [MagicMock(string='Verse 1'), MagicMock(string='Chorus'), MagicMock(string='Verse 2')] |
195 | + [ |
196 | + MagicMock(string='Verse 1'), |
197 | + MagicMock(string='Chorus'), |
198 | + MagicMock(string='Verse 2') |
199 | + ] |
200 | ] |
201 | mocked_lyrics_page.find.return_value = MagicMock(find_all=mocked_find_all) |
202 | MockedBeautifulSoup.side_effect = [mocked_song_page, mocked_lyrics_page] |
203 | @@ -349,8 +443,13 @@ |
204 | result = importer.get_song(fake_song, callback=mocked_callback) |
205 | |
206 | # THEN: The callback should have been called three times and the song should be returned |
207 | + mocked_song_page.find_all.assert_called_once_with('ul', 'song-meta-list') |
208 | + self.assertEqual(2, mocked_ul_copyright.find.call_count) |
209 | + self.assertEqual(1, mocked_ul_copyright.find_all.call_count) |
210 | + self.assertEqual(2, mocked_ul_themes.find.call_count) |
211 | + self.assertEqual(1, mocked_ul_themes.find_all.call_count) |
212 | self.assertEqual(3, mocked_callback.call_count, 'The callback should have been called twice') |
213 | - self.assertIsNotNone(result, 'The get_song() method should have returned a song dictionary') |
214 | + self.assertIsInstance(result, dict, 'The get_song() method should have returned a song dictionary') |
215 | self.assertEqual(2, mocked_lyrics_page.find.call_count, 'The find() method should have been called twice') |
216 | self.assertEqual(2, mocked_find_all.call_count, 'The find_all() method should have been called twice') |
217 | self.assertEqual([call('div', 'song-viewer lyrics'), call('div', 'song-viewer lyrics')], |
218 | @@ -359,7 +458,9 @@ |
219 | self.assertEqual([call('p'), call('h3')], mocked_find_all.call_args_list, |
220 | 'The find_all() method should have been called with the right arguments') |
221 | self.assertIn('copyright', result, 'The returned song should have a copyright') |
222 | + self.assertEqual('Copyright 1/Copyright 2', result['copyright']) |
223 | self.assertIn('ccli_number', result, 'The returned song should have a CCLI number') |
224 | + # self.assertEqual('123456', result['ccli_number'], result['ccli_number']) |
225 | self.assertIn('verses', result, 'The returned song should have verses') |
226 | self.assertEqual(3, len(result['verses']), 'Three verses should have been returned') |
227 | |
228 | @@ -375,7 +476,7 @@ |
229 | 'authors': ['Public Domain'], |
230 | 'verses': [ |
231 | {'label': 'Verse 1', 'lyrics': 'The Lord told Noah: there\'s gonna be a floody, floody'}, |
232 | - {'label': 'Chorus 1', 'lyrics': 'So, rise and shine, and give God the glory, glory'}, |
233 | + {'label': 'Chorus', 'lyrics': 'So, rise and shine, and give God the glory, glory'}, |
234 | {'label': 'Verse 2', 'lyrics': 'The Lord told Noah to build him an arky, arky'} |
235 | ], |
236 | 'copyright': 'Public Domain', |
237 | @@ -435,9 +536,8 @@ |
238 | self.assertEqual(1, len(result.authors_songs), 'There should only be one author') |
239 | |
240 | @patch('openlp.plugins.songs.lib.songselect.clean_song') |
241 | - @patch('openlp.plugins.songs.lib.songselect.Topic') |
242 | @patch('openlp.plugins.songs.lib.songselect.Author') |
243 | - def save_song_unknown_author_test(self, MockedAuthor, MockedTopic, mocked_clean_song): |
244 | + def save_song_unknown_author_test(self, MockedAuthor, mocked_clean_song): |
245 | """ |
246 | Test that saving a song with an author name of only one word performs the correct actions |
247 | """ |
248 | @@ -454,7 +554,6 @@ |
249 | 'ccli_number': '123456' |
250 | } |
251 | MockedAuthor.display_name.__eq__.return_value = False |
252 | - MockedTopic.name.__eq__.return_value = False |
253 | mocked_db_manager = MagicMock() |
254 | mocked_db_manager.get_object_filtered.return_value = None |
255 | importer = SongSelectImport(mocked_db_manager) |
256 | @@ -471,6 +570,45 @@ |
257 | MockedAuthor.populate.assert_called_with(first_name='Unknown', last_name='', |
258 | display_name='Unknown') |
259 | self.assertEqual(1, len(result.authors_songs), 'There should only be one author') |
260 | + # self.assertEqual(2, len(result.topics), 'There should only be two topics') |
261 | + |
262 | + @patch('openlp.plugins.songs.lib.songselect.clean_song') |
263 | + @patch('openlp.plugins.songs.lib.songselect.Topic') |
264 | + @patch('openlp.plugins.songs.lib.songselect.Author') |
265 | + def save_song_topics_test(self, MockedAuthor, MockedTopic, mocked_clean_song): |
266 | + """ |
267 | + Test that saving a song with topics performs the correct actions |
268 | + """ |
269 | + # GIVEN: A song to save, and some mocked out objects |
270 | + song_dict = { |
271 | + 'title': 'Arky Arky', |
272 | + 'authors': ['Public Domain'], |
273 | + 'verses': [ |
274 | + {'label': 'Verse 1', 'lyrics': 'The Lord told Noah: there\'s gonna be a floody, floody'}, |
275 | + {'label': 'Chorus', 'lyrics': 'So, rise and shine, and give God the glory, glory'}, |
276 | + {'label': 'Verse 2', 'lyrics': 'The Lord told Noah to build him an arky, arky'} |
277 | + ], |
278 | + 'copyright': 'Public Domain', |
279 | + 'ccli_number': '123456', |
280 | + 'topics': ['Grace', 'Love'] |
281 | + } |
282 | + MockedAuthor.display_name.__eq__.return_value = False |
283 | + MockedTopic.name.__eq__.return_value = False |
284 | + mocked_db_manager = MagicMock() |
285 | + mocked_db_manager.get_object_filtered.return_value = None |
286 | + importer = SongSelectImport(mocked_db_manager) |
287 | + |
288 | + # WHEN: The song is saved to the database |
289 | + result = importer.save_song(song_dict) |
290 | + |
291 | + # THEN: The return value should be a Song class and the mocked_db_manager should have been called |
292 | + self.assertIsInstance(result, Song, 'The returned value should be a Song object') |
293 | + mocked_clean_song.assert_called_with(mocked_db_manager, result) |
294 | + self.assertEqual(2, mocked_db_manager.save_object.call_count, |
295 | + 'The save_object() method should have been called twice') |
296 | + mocked_db_manager.get_object_filtered.assert_called_with(MockedTopic, False) |
297 | + self.assertEqual([call(name='Grace'), call(name='Love')], MockedTopic.populate.call_args_list) |
298 | + self.assertEqual(2, len(result.topics), 'There should be two topics') |
299 | |
300 | |
301 | class TestSongSelectForm(TestCase, TestMixin): |