Merge lp:~raoul-snyman/openlp/bug-1608194-2.4 into lp:openlp/2.4

Proposed by Raoul Snyman on 2016-08-12
Status: Merged
Merged at revision: 2648
Proposed branch: lp:~raoul-snyman/openlp/bug-1608194-2.4
Merge into: lp:openlp/2.4
Diff against target: 394 lines (+115/-62)
4 files modified
openlp/plugins/songs/lib/__init__.py (+2/-3)
openlp/plugins/songs/lib/db.py (+2/-2)
openlp/plugins/songs/lib/songselect.py (+72/-36)
tests/functional/openlp_plugins/songs/test_songselect.py (+39/-21)
To merge this branch: bzr merge lp:~raoul-snyman/openlp/bug-1608194-2.4
Reviewer Review Type Date Requested Status
Tim Bentley 2016-08-12 Approve on 2016-08-12
Review via email: mp+302797@code.launchpad.net

This proposal supersedes a proposal from 2016-08-11.

To post a comment you must log in.
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal

Update tests count.

You wrote the rules, I am just imposing them!

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/__init__.py'
2--- openlp/plugins/songs/lib/__init__.py 2016-04-21 19:49:22 +0000
3+++ openlp/plugins/songs/lib/__init__.py 2016-08-12 12:20:33 +0000
4@@ -32,9 +32,8 @@
5 from openlp.core.common import AppLocation
6 from openlp.core.lib import translate
7 from openlp.core.utils import CONTROL_CHARS
8-from openlp.plugins.songs.lib.db import MediaFile, Song
9-from .db import Author
10-from .ui import SongStrings
11+from openlp.plugins.songs.lib.db import Author, MediaFile, Song, Topic
12+from openlp.plugins.songs.lib.ui import SongStrings
13
14 log = logging.getLogger(__name__)
15
16
17=== modified file 'openlp/plugins/songs/lib/db.py'
18--- openlp/plugins/songs/lib/db.py 2016-04-27 18:45:39 +0000
19+++ openlp/plugins/songs/lib/db.py 2016-08-12 12:20:33 +0000
20@@ -135,7 +135,7 @@
21
22 def add_author(self, author, author_type=None):
23 """
24- Add an author to the song if it not yet exists
25+ Add an author to the song if it doesn't exist yet
26
27 :param author: Author object
28 :param author_type: AuthorType constant or None
29@@ -162,7 +162,7 @@
30
31 def add_songbook_entry(self, songbook, entry):
32 """
33- Add a Songbook Entry to the song if it not yet exists
34+ Add a Songbook Entry to the song if it doesn't exist yet
35
36 :param songbook_name: Name of the Songbook.
37 :param entry: Entry in the Songbook (usually a number)
38
39=== modified file 'openlp/plugins/songs/lib/songselect.py'
40--- openlp/plugins/songs/lib/songselect.py 2016-01-09 18:01:49 +0000
41+++ openlp/plugins/songs/lib/songselect.py 2016-08-12 12:20:33 +0000
42@@ -24,6 +24,8 @@
43 """
44 import logging
45 import sys
46+import random
47+import re
48 from http.cookiejar import CookieJar
49 from urllib.parse import urlencode
50 from urllib.request import HTTPCookieProcessor, URLError, build_opener
51@@ -32,14 +34,21 @@
52
53 from bs4 import BeautifulSoup, NavigableString
54
55-from openlp.plugins.songs.lib import Song, VerseType, clean_song, Author
56+from openlp.plugins.songs.lib import Song, Author, Topic, VerseType, clean_song
57 from openlp.plugins.songs.lib.openlyricsxml import SongXML
58
59-USER_AGENT = 'Mozilla/5.0 (Linux; U; Android 4.0.3; en-us; GT-I9000 ' \
60- 'Build/IML74K) AppleWebKit/534.30 (KHTML, like Gecko) Version/4.0 ' \
61- 'Mobile Safari/534.30'
62-BASE_URL = 'https://mobile.songselect.com'
63-LOGIN_URL = BASE_URL + '/account/login'
64+USER_AGENTS = [
65+ 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) '
66+ 'Chrome/52.0.2743.116 Safari/537.36',
67+ 'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.82 Safari/537.36',
68+ 'Mozilla/5.0 (X11; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0',
69+ 'Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:46.0) Gecko/20100101 Firefox/46.0',
70+ 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:47.0) Gecko/20100101 Firefox/47.0'
71+]
72+BASE_URL = 'https://songselect.ccli.com'
73+LOGIN_PAGE = 'https://profile.ccli.com/account/signin?appContext=SongSelect&returnUrl=' \
74+ 'https%3a%2f%2fsongselect.ccli.com%2f'
75+LOGIN_URL = 'https://profile.ccli.com/'
76 LOGOUT_URL = BASE_URL + '/account/logout'
77 SEARCH_URL = BASE_URL + '/search/results'
78
79@@ -60,7 +69,7 @@
80 self.db_manager = db_manager
81 self.html_parser = HTMLParser()
82 self.opener = build_opener(HTTPCookieProcessor(CookieJar()))
83- self.opener.addheaders = [('User-Agent', USER_AGENT)]
84+ self.opener.addheaders = [('User-Agent', random.choice(USER_AGENTS))]
85 self.run_search = True
86
87 def login(self, username, password, callback=None):
88@@ -76,27 +85,27 @@
89 if callback:
90 callback()
91 try:
92- login_page = BeautifulSoup(self.opener.open(LOGIN_URL).read(), 'lxml')
93- except (TypeError, URLError) as e:
94- log.exception('Could not login to SongSelect, %s', e)
95+ login_page = BeautifulSoup(self.opener.open(LOGIN_PAGE).read(), 'lxml')
96+ except (TypeError, URLError) as error:
97+ log.exception('Could not login to SongSelect, %s', error)
98 return False
99 if callback:
100 callback()
101 token_input = login_page.find('input', attrs={'name': '__RequestVerificationToken'})
102 data = urlencode({
103 '__RequestVerificationToken': token_input['value'],
104- 'UserName': username,
105- 'Password': password,
106+ 'emailAddress': username,
107+ 'password': password,
108 'RememberMe': 'false'
109 })
110 try:
111 posted_page = BeautifulSoup(self.opener.open(LOGIN_URL, data.encode('utf-8')).read(), 'lxml')
112- except (TypeError, URLError) as e:
113- log.exception('Could not login to SongSelect, %s', e)
114+ except (TypeError, URLError) as error:
115+ log.exception('Could not login to SongSelect, %s', error)
116 return False
117 if callback:
118 callback()
119- return not posted_page.find('input', attrs={'name': '__RequestVerificationToken'})
120+ return posted_page.find('input', id='SearchText') is not None
121
122 def logout(self):
123 """
124@@ -104,8 +113,8 @@
125 """
126 try:
127 self.opener.open(LOGOUT_URL)
128- except (TypeError, URLError) as e:
129- log.exception('Could not log of SongSelect, %s', e)
130+ except (TypeError, URLError) as error:
131+ log.exception('Could not log of SongSelect, %s', error)
132
133 def search(self, search_text, max_results, callback=None):
134 """
135@@ -117,7 +126,15 @@
136 :return: List of songs
137 """
138 self.run_search = True
139- params = {'allowredirect': 'false', 'SearchTerm': search_text}
140+ params = {
141+ 'SongContent': '',
142+ 'PrimaryLanguage': '',
143+ 'Keys': '',
144+ 'Themes': '',
145+ 'List': '',
146+ 'Sort': '',
147+ 'SearchText': search_text
148+ }
149 current_page = 1
150 songs = []
151 while self.run_search:
152@@ -125,7 +142,7 @@
153 params['page'] = current_page
154 try:
155 results_page = BeautifulSoup(self.opener.open(SEARCH_URL + '?' + urlencode(params)).read(), 'lxml')
156- search_results = results_page.find_all('li', 'result pane')
157+ search_results = results_page.find_all('div', 'song-result')
158 except (TypeError, URLError) as e:
159 log.exception('Could not search SongSelect, %s', e)
160 search_results = None
161@@ -133,9 +150,9 @@
162 break
163 for result in search_results:
164 song = {
165- 'title': unescape(result.find('h3').string),
166- 'authors': [unescape(author.string) for author in result.find_all('li')],
167- 'link': BASE_URL + result.find('a')['href']
168+ 'title': unescape(result.find('p', 'song-result-title').find('a').string).strip(),
169+ 'authors': unescape(result.find('p', 'song-result-subtitle').string).strip().split(', '),
170+ 'link': BASE_URL + result.find('p', 'song-result-title').find('a')['href']
171 }
172 if callback:
173 callback(song)
174@@ -163,27 +180,37 @@
175 if callback:
176 callback()
177 try:
178- lyrics_page = BeautifulSoup(self.opener.open(song['link'] + '/lyrics').read(), 'lxml')
179+ lyrics_page = BeautifulSoup(self.opener.open(song['link'] + '/viewlyrics').read(), 'lxml')
180 except (TypeError, URLError):
181 log.exception('Could not get lyrics from SongSelect')
182 return None
183 if callback:
184 callback()
185- song['copyright'] = '/'.join([li.string for li in song_page.find('ul', 'copyright').find_all('li')])
186- song['copyright'] = unescape(song['copyright'])
187- song['ccli_number'] = song_page.find('ul', 'info').find('li').string.split(':')[1].strip()
188+ copyright_elements = []
189+ theme_elements = []
190+ copyrights_regex = re.compile(r'\bCopyrights\b')
191+ themes_regex = re.compile(r'\bThemes\b')
192+ for ul in song_page.find_all('ul', 'song-meta-list'):
193+ if ul.find('li', string=copyrights_regex):
194+ copyright_elements.extend(ul.find_all('li')[1:])
195+ if ul.find('li', string=themes_regex):
196+ theme_elements.extend(ul.find_all('li')[1:])
197+ song['copyright'] = '/'.join([unescape(li.string).strip() for li in copyright_elements])
198+ song['topics'] = [unescape(li.string).strip() for li in theme_elements]
199+ song['ccli_number'] = song_page.find('div', 'song-content-data').find('ul').find('li')\
200+ .find('strong').string.strip()
201 song['verses'] = []
202- verses = lyrics_page.find('section', 'lyrics').find_all('p')
203- verse_labels = lyrics_page.find('section', 'lyrics').find_all('h3')
204- for counter in range(len(verses)):
205- verse = {'label': verse_labels[counter].string, 'lyrics': ''}
206- for v in verses[counter].contents:
207+ verses = lyrics_page.find('div', 'song-viewer lyrics').find_all('p')
208+ verse_labels = lyrics_page.find('div', 'song-viewer lyrics').find_all('h3')
209+ for verse, label in zip(verses, verse_labels):
210+ song_verse = {'label': unescape(label.string).strip(), 'lyrics': ''}
211+ for v in verse.contents:
212 if isinstance(v, NavigableString):
213- verse['lyrics'] = verse['lyrics'] + v.string
214+ song_verse['lyrics'] += unescape(v.string).strip()
215 else:
216- verse['lyrics'] += '\n'
217- verse['lyrics'] = verse['lyrics'].strip(' \n\r\t')
218- song['verses'].append(unescape(verse))
219+ song_verse['lyrics'] += '\n'
220+ song_verse['lyrics'] = song_verse['lyrics'].strip()
221+ song['verses'].append(song_verse)
222 for counter, author in enumerate(song['authors']):
223 song['authors'][counter] = unescape(author)
224 return song
225@@ -199,7 +226,11 @@
226 song_xml = SongXML()
227 verse_order = []
228 for verse in song['verses']:
229- verse_type, verse_number = verse['label'].split(' ')[:2]
230+ if ' ' in verse['label']:
231+ verse_type, verse_number = verse['label'].split(' ', 1)
232+ else:
233+ verse_type = verse['label']
234+ verse_number = 1
235 verse_type = VerseType.from_loose_input(verse_type)
236 verse_number = int(verse_number)
237 song_xml.add_verse_to_lyrics(VerseType.tags[verse_type], verse_number, verse['lyrics'])
238@@ -220,6 +251,11 @@
239 last_name = name_parts[1]
240 author = Author.populate(first_name=first_name, last_name=last_name, display_name=author_name)
241 db_song.add_author(author)
242+ for topic_name in song.get('topics', []):
243+ topic = self.db_manager.get_object_filtered(Topic, Topic.name == topic_name)
244+ if not topic:
245+ topic = Topic.populate(name=topic_name)
246+ db_song.add_topic(topic)
247 self.db_manager.save_object(db_song)
248 return db_song
249
250
251=== modified file 'tests/functional/openlp_plugins/songs/test_songselect.py'
252--- tests/functional/openlp_plugins/songs/test_songselect.py 2016-01-09 18:01:49 +0000
253+++ tests/functional/openlp_plugins/songs/test_songselect.py 2016-08-12 12:20:33 +0000
254@@ -71,7 +71,7 @@
255 mocked_opener = MagicMock()
256 mocked_build_opener.return_value = mocked_opener
257 mocked_login_page = MagicMock()
258- mocked_login_page.find.return_value = {'value': 'blah'}
259+ mocked_login_page.find.side_effect = [{'value': 'blah'}, None]
260 MockedBeautifulSoup.return_value = mocked_login_page
261 mock_callback = MagicMock()
262 importer = SongSelectImport(None)
263@@ -112,7 +112,7 @@
264 mocked_opener = MagicMock()
265 mocked_build_opener.return_value = mocked_opener
266 mocked_login_page = MagicMock()
267- mocked_login_page.find.side_effect = [{'value': 'blah'}, None]
268+ mocked_login_page.find.side_effect = [{'value': 'blah'}, MagicMock()]
269 MockedBeautifulSoup.return_value = mocked_login_page
270 mock_callback = MagicMock()
271 importer = SongSelectImport(None)
272@@ -165,7 +165,7 @@
273 self.assertEqual(0, mock_callback.call_count, 'callback should not have been called')
274 self.assertEqual(1, mocked_opener.open.call_count, 'open should have been called once')
275 self.assertEqual(1, mocked_results_page.find_all.call_count, 'find_all should have been called once')
276- mocked_results_page.find_all.assert_called_with('li', 'result pane')
277+ mocked_results_page.find_all.assert_called_with('div', 'song-result')
278 self.assertEqual([], results, 'The search method should have returned an empty list')
279
280 @patch('openlp.plugins.songs.lib.songselect.build_opener')
281@@ -177,12 +177,18 @@
282 # GIVEN: A bunch of mocked out stuff and an importer object
283 # first search result
284 mocked_result1 = MagicMock()
285- mocked_result1.find.side_effect = [MagicMock(string='Title 1'), {'href': '/url1'}]
286- mocked_result1.find_all.return_value = [MagicMock(string='Author 1-1'), MagicMock(string='Author 1-2')]
287+ mocked_result1.find.side_effect = [
288+ MagicMock(find=MagicMock(return_value=MagicMock(string='Title 1'))),
289+ MagicMock(string='James, John'),
290+ MagicMock(find=MagicMock(return_value={'href': '/url1'}))
291+ ]
292 # second search result
293 mocked_result2 = MagicMock()
294- mocked_result2.find.side_effect = [MagicMock(string='Title 2'), {'href': '/url2'}]
295- mocked_result2.find_all.return_value = [MagicMock(string='Author 2-1'), MagicMock(string='Author 2-2')]
296+ mocked_result2.find.side_effect = [
297+ MagicMock(find=MagicMock(return_value=MagicMock(string='Title 2'))),
298+ MagicMock(string='Philip'),
299+ MagicMock(find=MagicMock(return_value={'href': '/url2'}))
300+ ]
301 # rest of the stuff
302 mocked_opener = MagicMock()
303 mocked_build_opener.return_value = mocked_opener
304@@ -196,13 +202,14 @@
305 results = importer.search('text', 1000, mock_callback)
306
307 # THEN: callback was never called, open was called once, find_all was called once, an empty list returned
308+ self.maxDiff = None
309 self.assertEqual(2, mock_callback.call_count, 'callback should have been called twice')
310 self.assertEqual(2, mocked_opener.open.call_count, 'open should have been called twice')
311 self.assertEqual(2, mocked_results_page.find_all.call_count, 'find_all should have been called twice')
312- mocked_results_page.find_all.assert_called_with('li', 'result pane')
313+ mocked_results_page.find_all.assert_called_with('div', 'song-result')
314 expected_list = [
315- {'title': 'Title 1', 'authors': ['Author 1-1', 'Author 1-2'], 'link': BASE_URL + '/url1'},
316- {'title': 'Title 2', 'authors': ['Author 2-1', 'Author 2-2'], 'link': BASE_URL + '/url2'}
317+ {'title': 'Title 1', 'authors': ['James', 'John'], 'link': BASE_URL + '/url1'},
318+ {'title': 'Title 2', 'authors': ['Philip'], 'link': BASE_URL + '/url2'}
319 ]
320 self.assertListEqual(expected_list, results, 'The search method should have returned two songs')
321
322@@ -215,16 +222,25 @@
323 # GIVEN: A bunch of mocked out stuff and an importer object
324 # first search result
325 mocked_result1 = MagicMock()
326- mocked_result1.find.side_effect = [MagicMock(string='Title 1'), {'href': '/url1'}]
327- mocked_result1.find_all.return_value = [MagicMock(string='Author 1-1'), MagicMock(string='Author 1-2')]
328+ mocked_result1.find.side_effect = [
329+ MagicMock(find=MagicMock(return_value=MagicMock(string='Title 1'))),
330+ MagicMock(string='James, John'),
331+ MagicMock(find=MagicMock(return_value={'href': '/url1'}))
332+ ]
333 # second search result
334 mocked_result2 = MagicMock()
335- mocked_result2.find.side_effect = [MagicMock(string='Title 2'), {'href': '/url2'}]
336- mocked_result2.find_all.return_value = [MagicMock(string='Author 2-1'), MagicMock(string='Author 2-2')]
337+ mocked_result2.find.side_effect = [
338+ MagicMock(find=MagicMock(return_value=MagicMock(string='Title 2'))),
339+ MagicMock(string='Philip'),
340+ MagicMock(find=MagicMock(return_value={'href': '/url2'}))
341+ ]
342 # third search result
343 mocked_result3 = MagicMock()
344- mocked_result3.find.side_effect = [MagicMock(string='Title 3'), {'href': '/url3'}]
345- mocked_result3.find_all.return_value = [MagicMock(string='Author 3-1'), MagicMock(string='Author 3-2')]
346+ mocked_result3.find.side_effect = [
347+ MagicMock(find=MagicMock(return_value=MagicMock(string='Title 3'))),
348+ MagicMock(string='Luke, Matthew'),
349+ MagicMock(find=MagicMock(return_value={'href': '/url3'}))
350+ ]
351 # rest of the stuff
352 mocked_opener = MagicMock()
353 mocked_build_opener.return_value = mocked_opener
354@@ -241,9 +257,9 @@
355 self.assertEqual(2, mock_callback.call_count, 'callback should have been called twice')
356 self.assertEqual(2, mocked_opener.open.call_count, 'open should have been called twice')
357 self.assertEqual(2, mocked_results_page.find_all.call_count, 'find_all should have been called twice')
358- mocked_results_page.find_all.assert_called_with('li', 'result pane')
359- expected_list = [{'title': 'Title 1', 'authors': ['Author 1-1', 'Author 1-2'], 'link': BASE_URL + '/url1'},
360- {'title': 'Title 2', 'authors': ['Author 2-1', 'Author 2-2'], 'link': BASE_URL + '/url2'}]
361+ mocked_results_page.find_all.assert_called_with('div', 'song-result')
362+ expected_list = [{'title': 'Title 1', 'authors': ['James', 'John'], 'link': BASE_URL + '/url1'},
363+ {'title': 'Title 2', 'authors': ['Philip'], 'link': BASE_URL + '/url2'}]
364 self.assertListEqual(expected_list, results, 'The search method should have returned two songs')
365
366 @patch('openlp.plugins.songs.lib.songselect.build_opener')
367@@ -337,7 +353,7 @@
368 self.assertIsNotNone(result, 'The get_song() method should have returned a song dictionary')
369 self.assertEqual(2, mocked_lyrics_page.find.call_count, 'The find() method should have been called twice')
370 self.assertEqual(2, mocked_find_all.call_count, 'The find_all() method should have been called twice')
371- self.assertEqual([call('section', 'lyrics'), call('section', 'lyrics')],
372+ self.assertEqual([call('div', 'song-viewer lyrics'), call('div', 'song-viewer lyrics')],
373 mocked_lyrics_page.find.call_args_list,
374 'The find() method should have been called with the right arguments')
375 self.assertEqual([call('p'), call('h3')], mocked_find_all.call_args_list,
376@@ -419,8 +435,9 @@
377 self.assertEqual(1, len(result.authors_songs), 'There should only be one author')
378
379 @patch('openlp.plugins.songs.lib.songselect.clean_song')
380+ @patch('openlp.plugins.songs.lib.songselect.Topic')
381 @patch('openlp.plugins.songs.lib.songselect.Author')
382- def save_song_unknown_author_test(self, MockedAuthor, mocked_clean_song):
383+ def save_song_unknown_author_test(self, MockedAuthor, MockedTopic, mocked_clean_song):
384 """
385 Test that saving a song with an author name of only one word performs the correct actions
386 """
387@@ -437,6 +454,7 @@
388 'ccli_number': '123456'
389 }
390 MockedAuthor.display_name.__eq__.return_value = False
391+ MockedTopic.name.__eq__.return_value = False
392 mocked_db_manager = MagicMock()
393 mocked_db_manager.get_object_filtered.return_value = None
394 importer = SongSelectImport(mocked_db_manager)

Subscribers

People subscribed via source and target branches

to all changes: