Merge lp:~raoul-snyman/openlp/fix-importers-2.4 into lp:openlp/2.4

Proposed by Raoul Snyman
Status: Merged
Merged at revision: 2675
Proposed branch: lp:~raoul-snyman/openlp/fix-importers-2.4
Merge into: lp:openlp/2.4
Diff against target: 420 lines (+149/-54)
9 files modified
openlp/core/__init__.py (+1/-1)
openlp/plugins/songs/lib/importers/openlp.py (+8/-2)
openlp/plugins/songs/lib/importers/presentationmanager.py (+33/-10)
openlp/plugins/songs/lib/songselect.py (+16/-3)
tests/functional/openlp_core/test_init.py (+32/-18)
tests/functional/openlp_plugins/remotes/test_router.py (+9/-5)
tests/functional/openlp_plugins/songs/test_songselect.py (+42/-7)
tests/interfaces/openlp_plugins/bibles/forms/test_bibleimportform.py (+7/-7)
tests/resources/presentationmanagersongs/Agnus Dei.sng (+1/-1)
To merge this branch: bzr merge lp:~raoul-snyman/openlp/fix-importers-2.4
Reviewer Review Type Date Requested Status
Tim Bentley Approve
Review via email: mp+318838@code.launchpad.net

This proposal supersedes a proposal from 2017-03-02.

Description of the change

- Fix SongSelect so that it detects the login URL
- Fix PresentationManager importer to handle weird XML
- Pull in OpenLP song importer fixes from Olli's branch

To post a comment you must log in.
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/core/__init__.py'
2--- openlp/core/__init__.py 2017-01-22 20:01:53 +0000
3+++ openlp/core/__init__.py 2017-03-02 21:00:50 +0000
4@@ -317,7 +317,7 @@
5 return QtWidgets.QApplication.event(self, event)
6
7
8-def parse_options(args):
9+def parse_options(args=None):
10 """
11 Parse the command line arguments
12
13
14=== modified file 'openlp/plugins/songs/lib/importers/openlp.py'
15--- openlp/plugins/songs/lib/importers/openlp.py 2016-12-31 11:05:48 +0000
16+++ openlp/plugins/songs/lib/importers/openlp.py 2017-03-02 21:00:50 +0000
17@@ -221,11 +221,17 @@
18 if not existing_book:
19 existing_book = Book.populate(name=entry.songbook.name, publisher=entry.songbook.publisher)
20 new_song.add_songbook_entry(existing_book, entry.entry)
21- elif song.book:
22+ elif hasattr(song, 'book') and song.book:
23 existing_book = self.manager.get_object_filtered(Book, Book.name == song.book.name)
24 if not existing_book:
25 existing_book = Book.populate(name=song.book.name, publisher=song.book.publisher)
26- new_song.add_songbook_entry(existing_book, '')
27+ # Get the song_number from "songs" table "song_number" field. (This is db structure from 2.2.1)
28+ # If there's a number, add it to the song, otherwise it will be "".
29+ existing_number = song.song_number if hasattr(song, 'song_number') else ''
30+ if existing_number:
31+ new_song.add_songbook_entry(existing_book, existing_number)
32+ else:
33+ new_song.add_songbook_entry(existing_book, "")
34 # Find or create all the media files and add them to the new song object
35 if has_media_files and song.media_files:
36 for media_file in song.media_files:
37
38=== modified file 'openlp/plugins/songs/lib/importers/presentationmanager.py'
39--- openlp/plugins/songs/lib/importers/presentationmanager.py 2016-12-31 11:05:48 +0000
40+++ openlp/plugins/songs/lib/importers/presentationmanager.py 2017-03-02 21:00:50 +0000
41@@ -23,7 +23,7 @@
42 The :mod:`presentationmanager` module provides the functionality for importing
43 Presentationmanager song files into the current database.
44 """
45-
46+import logging
47 import os
48 import re
49 import chardet
50@@ -32,6 +32,8 @@
51 from openlp.core.ui.wizard import WizardStrings
52 from .songimport import SongImport
53
54+log = logging.getLogger(__name__)
55+
56
57 class PresentationManagerImport(SongImport):
58 """
59@@ -62,15 +64,36 @@
60 'File is not in XML-format, which is the only format supported.'))
61 continue
62 root = objectify.fromstring(etree.tostring(tree))
63- self.process_song(root)
64-
65- def process_song(self, root):
66+ self.process_song(root, file_path)
67+
68+ def _get_attr(self, elem, name):
69+ """
70+ Due to PresentationManager's habit of sometimes capitilising the first letter of an element, we have to do
71+ some gymnastics.
72+ """
73+ if hasattr(elem, name):
74+ log.debug('%s: %s', name, getattr(elem, name))
75+ return str(getattr(elem, name))
76+ name = name[0].upper() + name[1:]
77+ if hasattr(elem, name):
78+ log.debug('%s: %s', name, getattr(elem, name))
79+ return str(getattr(elem, name))
80+ else:
81+ return ''
82+
83+ def process_song(self, root, file_path):
84 self.set_defaults()
85- self.title = str(root.attributes.title)
86- self.add_author(str(root.attributes.author))
87- self.copyright = str(root.attributes.copyright)
88- self.ccli_number = str(root.attributes.ccli_number)
89- self.comments = str(root.attributes.comments)
90+ attrs = None
91+ if hasattr(root, 'attributes'):
92+ attrs = root.attributes
93+ elif hasattr(root, 'Attributes'):
94+ attrs = root.Attributes
95+ if attrs is not None:
96+ self.title = self._get_attr(root.attributes, 'title')
97+ self.add_author(self._get_attr(root.attributes, 'author'))
98+ self.copyright = self._get_attr(root.attributes, 'copyright')
99+ self.ccli_number = self._get_attr(root.attributes, 'ccli_number')
100+ self.comments = str(root.attributes.comments) if hasattr(root.attributes, 'comments') else None
101 verse_order_list = []
102 verse_count = {}
103 duplicates = []
104@@ -102,4 +125,4 @@
105
106 self.verse_order_list = verse_order_list
107 if not self.finish():
108- self.log_error(self.import_source)
109+ self.log_error(os.path.basename(file_path))
110
111=== modified file 'openlp/plugins/songs/lib/songselect.py'
112--- openlp/plugins/songs/lib/songselect.py 2016-12-31 11:05:48 +0000
113+++ openlp/plugins/songs/lib/songselect.py 2017-03-02 21:00:50 +0000
114@@ -47,7 +47,7 @@
115 BASE_URL = 'https://songselect.ccli.com'
116 LOGIN_PAGE = 'https://profile.ccli.com/account/signin?appContext=SongSelect&returnUrl=' \
117 'https%3a%2f%2fsongselect.ccli.com%2f'
118-LOGIN_URL = 'https://profile.ccli.com/'
119+LOGIN_URL = 'https://profile.ccli.com'
120 LOGOUT_URL = BASE_URL + '/account/logout'
121 SEARCH_URL = BASE_URL + '/search/results'
122
123@@ -97,14 +97,27 @@
124 'password': password,
125 'RememberMe': 'false'
126 })
127+ login_form = login_page.find('form')
128+ if login_form:
129+ login_url = login_form.attrs['action']
130+ else:
131+ login_url = '/Account/SignIn'
132+ if not login_url.startswith('http'):
133+ if login_url[0] != '/':
134+ login_url = '/' + login_url
135+ login_url = LOGIN_URL + login_url
136 try:
137- posted_page = BeautifulSoup(self.opener.open(LOGIN_URL, data.encode('utf-8')).read(), 'lxml')
138+ posted_page = BeautifulSoup(self.opener.open(login_url, data.encode('utf-8')).read(), 'lxml')
139 except (TypeError, URLError) as error:
140 log.exception('Could not login to SongSelect, %s', error)
141 return False
142 if callback:
143 callback()
144- return posted_page.find('input', id='SearchText') is not None
145+ if posted_page.find('input', id='SearchText') is not None:
146+ return True
147+ else:
148+ log.debug(posted_page)
149+ return False
150
151 def logout(self):
152 """
153
154=== added file 'tests/functional/openlp_core/__init__.py'
155=== modified file 'tests/functional/openlp_core/test_init.py'
156--- tests/functional/openlp_core/test_init.py 2016-12-31 11:05:48 +0000
157+++ tests/functional/openlp_core/test_init.py 2017-03-02 21:00:50 +0000
158@@ -22,12 +22,12 @@
159
160 import sys
161 from unittest import TestCase
162-
163-from openlp.core import parse_options
164-from tests.helpers.testmixin import TestMixin
165-
166-
167-class TestInitFunctions(TestMixin, TestCase):
168+from unittest.mock import MagicMock, patch
169+
170+from openlp.core import OpenLP, parse_options
171+
172+
173+class TestInitFunctions(TestCase):
174
175 def parse_options_basic_test(self):
176 """
177@@ -116,7 +116,7 @@
178
179 def parse_options_file_and_debug_test(self):
180 """
181- Test the parse options process works with a file
182+ Test the parse options process works with a file and the debug log level
183
184 """
185 # GIVEN: a a set of system arguments.
186@@ -131,14 +131,28 @@
187 self.assertEquals(args.style, None, 'There are no style flags to be processed')
188 self.assertEquals(args.rargs, 'dummy_temp', 'The service file should not be blank')
189
190- def parse_options_two_files_test(self):
191- """
192- Test the parse options process works with a file
193-
194- """
195- # GIVEN: a a set of system arguments.
196- sys.argv[1:] = ['dummy_temp', 'dummy_temp2']
197- # WHEN: We we parse them to expand to options
198- args = parse_options()
199- # THEN: the following fields will have been extracted.
200- self.assertEquals(args, None, 'The args should be None')
201+
202+class TestOpenLP(TestCase):
203+ """
204+ Test the OpenLP app class
205+ """
206+ @patch('openlp.core.QtWidgets.QApplication.exec')
207+ def test_exec(self, mocked_exec):
208+ """
209+ Test the exec method
210+ """
211+ # GIVEN: An app
212+ app = OpenLP([])
213+ app.shared_memory = MagicMock()
214+ mocked_exec.return_value = False
215+
216+ # WHEN: exec() is called
217+ result = app.exec()
218+
219+ # THEN: The right things should be called
220+ assert app.is_event_loop_active is True
221+ mocked_exec.assert_called_once_with()
222+ app.shared_memory.detach.assert_called_once_with()
223+ assert result is False
224+
225+ del app
226
227=== modified file 'tests/functional/openlp_plugins/remotes/test_router.py'
228--- tests/functional/openlp_plugins/remotes/test_router.py 2016-12-31 11:05:48 +0000
229+++ tests/functional/openlp_plugins/remotes/test_router.py 2017-03-02 21:00:50 +0000
230@@ -25,11 +25,11 @@
231 import os
232 import urllib.request
233 from unittest import TestCase
234+from unittest.mock import MagicMock, patch, mock_open
235
236 from openlp.core.common import Settings, Registry
237 from openlp.core.ui import ServiceManager
238 from openlp.plugins.remotes.lib.httpserver import HttpRouter
239-from tests.functional import MagicMock, patch, mock_open
240 from tests.helpers.testmixin import TestMixin
241
242 __default_settings__ = {
243@@ -313,11 +313,13 @@
244 with patch.object(self.service_manager, 'setup_ui'), \
245 patch.object(self.router, 'do_json_header'):
246 self.service_manager.bootstrap_initialise()
247- self.app.processEvents()
248+ # Not sure why this is here, it doesn't make sense in the test
249+ # self.app.processEvents()
250
251 # WHEN: Remote next is received
252 self.router.service(action='next')
253- self.app.processEvents()
254+ # Not sure why this is here, it doesn't make sense in the test
255+ # self.app.processEvents()
256
257 # THEN: service_manager.next_item() should have been called
258 self.assertTrue(mocked_next_item.called, 'next_item() should have been called in service_manager')
259@@ -334,11 +336,13 @@
260 with patch.object(self.service_manager, 'setup_ui'), \
261 patch.object(self.router, 'do_json_header'):
262 self.service_manager.bootstrap_initialise()
263- self.app.processEvents()
264+ # Not sure why this is here, it doesn't make sense in the test
265+ # self.app.processEvents()
266
267 # WHEN: Remote next is received
268 self.router.service(action='previous')
269- self.app.processEvents()
270+ # Not sure why this is here, it doesn't make sense in the test
271+ # self.app.processEvents()
272
273 # THEN: service_manager.next_item() should have been called
274 self.assertTrue(mocked_previous_item.called, 'previous_item() should have been called in service_manager')
275
276=== modified file 'tests/functional/openlp_plugins/songs/test_songselect.py'
277--- tests/functional/openlp_plugins/songs/test_songselect.py 2016-12-31 11:05:48 +0000
278+++ tests/functional/openlp_plugins/songs/test_songselect.py 2017-03-02 21:00:50 +0000
279@@ -32,7 +32,7 @@
280 from openlp.core import Registry
281 from openlp.plugins.songs.forms.songselectform import SongSelectForm, SearchWorker
282 from openlp.plugins.songs.lib import Song
283-from openlp.plugins.songs.lib.songselect import SongSelectImport, LOGOUT_URL, BASE_URL
284+from openlp.plugins.songs.lib.songselect import SongSelectImport, LOGIN_PAGE, LOGOUT_URL, BASE_URL
285
286 from tests.functional import MagicMock, patch, call
287 from tests.helpers.songfileimport import SongImportTestHelper
288@@ -81,7 +81,7 @@
289
290 # THEN: callback was called 3 times, open was called twice, find was called twice, and False was returned
291 self.assertEqual(2, mock_callback.call_count, 'callback should have been called 3 times')
292- self.assertEqual(1, mocked_login_page.find.call_count, 'find should have been called twice')
293+ self.assertEqual(2, mocked_login_page.find.call_count, 'find should have been called twice')
294 self.assertEqual(2, mocked_opener.open.call_count, 'opener should have been called twice')
295 self.assertFalse(result, 'The login method should have returned False')
296
297@@ -96,7 +96,9 @@
298 mocked_build_opener.return_value = mocked_opener
299 mocked_login_page = MagicMock()
300 mocked_login_page.find.side_effect = [{'value': 'blah'}, None]
301- MockedBeautifulSoup.return_value = mocked_login_page
302+ mocked_posted_page = MagicMock()
303+ mocked_posted_page.find.return_value = None
304+ MockedBeautifulSoup.side_effect = [mocked_login_page, mocked_posted_page]
305 mock_callback = MagicMock()
306 importer = SongSelectImport(None)
307
308@@ -105,7 +107,8 @@
309
310 # THEN: callback was called 3 times, open was called twice, find was called twice, and False was returned
311 self.assertEqual(3, mock_callback.call_count, 'callback should have been called 3 times')
312- self.assertEqual(2, mocked_login_page.find.call_count, 'find should have been called twice')
313+ self.assertEqual(2, mocked_login_page.find.call_count, 'find should have been called twice on the login page')
314+ self.assertEqual(1, mocked_posted_page.find.call_count, 'find should have been called once on the posted page')
315 self.assertEqual(2, mocked_opener.open.call_count, 'opener should have been called twice')
316 self.assertFalse(result, 'The login method should have returned False')
317
318@@ -136,8 +139,10 @@
319 mocked_opener = MagicMock()
320 mocked_build_opener.return_value = mocked_opener
321 mocked_login_page = MagicMock()
322- mocked_login_page.find.side_effect = [{'value': 'blah'}, MagicMock()]
323- MockedBeautifulSoup.return_value = mocked_login_page
324+ mocked_login_page.find.side_effect = [{'value': 'blah'}, None]
325+ mocked_posted_page = MagicMock()
326+ mocked_posted_page.find.return_value = MagicMock()
327+ MockedBeautifulSoup.side_effect = [mocked_login_page, mocked_posted_page]
328 mock_callback = MagicMock()
329 importer = SongSelectImport(None)
330
331@@ -146,11 +151,41 @@
332
333 # THEN: callback was called 3 times, open was called twice, find was called twice, and True was returned
334 self.assertEqual(3, mock_callback.call_count, 'callback should have been called 3 times')
335- self.assertEqual(2, mocked_login_page.find.call_count, 'find should have been called twice')
336+ self.assertEqual(2, mocked_login_page.find.call_count, 'find should have been called twice on the login page')
337+ self.assertEqual(1, mocked_posted_page.find.call_count, 'find should have been called once on the posted page')
338 self.assertEqual(2, mocked_opener.open.call_count, 'opener should have been called twice')
339 self.assertTrue(result, 'The login method should have returned True')
340
341 @patch('openlp.plugins.songs.lib.songselect.build_opener')
342+ @patch('openlp.plugins.songs.lib.songselect.BeautifulSoup')
343+ def login_url_from_form_test(self, MockedBeautifulSoup, mocked_build_opener):
344+ """
345+ Test that the login URL is from the form
346+ """
347+ # GIVEN: A bunch of mocked out stuff and an importer object
348+ mocked_opener = MagicMock()
349+ mocked_build_opener.return_value = mocked_opener
350+ mocked_form = MagicMock()
351+ mocked_form.attrs = {'action': 'do/login'}
352+ mocked_login_page = MagicMock()
353+ mocked_login_page.find.side_effect = [{'value': 'blah'}, mocked_form]
354+ mocked_posted_page = MagicMock()
355+ mocked_posted_page.find.return_value = MagicMock()
356+ MockedBeautifulSoup.side_effect = [mocked_login_page, mocked_posted_page]
357+ mock_callback = MagicMock()
358+ importer = SongSelectImport(None)
359+
360+ # WHEN: The login method is called after being rigged to fail
361+ result = importer.login('username', 'password', mock_callback)
362+
363+ # THEN: callback was called 3 times, open was called twice, find was called twice, and True was returned
364+ self.assertEqual(3, mock_callback.call_count, 'callback should have been called 3 times')
365+ self.assertEqual(2, mocked_login_page.find.call_count, 'find should have been called twice on the login page')
366+ self.assertEqual(1, mocked_posted_page.find.call_count, 'find should have been called once on the posted page')
367+ self.assertEqual('https://profile.ccli.com/do/login', mocked_opener.open.call_args_list[1][0][0])
368+ self.assertTrue(result, 'The login method should have returned True')
369+
370+ @patch('openlp.plugins.songs.lib.songselect.build_opener')
371 def logout_test(self, mocked_build_opener):
372 """
373 Test that when the logout method is called, it logs the user out of SongSelect
374
375=== added file 'tests/interfaces/openlp_plugins/bibles/forms/__init__.py'
376=== modified file 'tests/interfaces/openlp_plugins/bibles/forms/test_bibleimportform.py'
377--- tests/interfaces/openlp_plugins/bibles/forms/test_bibleimportform.py 2016-12-31 11:05:48 +0000
378+++ tests/interfaces/openlp_plugins/bibles/forms/test_bibleimportform.py 2017-03-02 21:00:50 +0000
379@@ -22,7 +22,7 @@
380 """
381 Package to test the openlp.plugins.bibles.forms.bibleimportform package.
382 """
383-from unittest import TestCase
384+from unittest import TestCase, skip
385
386 from PyQt5 import QtWidgets
387
388@@ -48,12 +48,12 @@
389 Registry().register('main_window', self.main_window)
390 self.form = BibleImportForm(self.main_window, MagicMock(), MagicMock())
391
392- def tearDown(self):
393- """
394- Delete all the C++ objects at the end so that we don't have a segfault
395- """
396- del self.form
397- del self.main_window
398+ # def tearDown(self):
399+ # """
400+ # Delete all the C++ objects at the end so that we don't have a segfault
401+ # """
402+ # del self.form
403+ # del self.main_window
404
405 @patch('openlp.plugins.bibles.forms.bibleimportform.CWExtract.get_bibles_from_http')
406 @patch('openlp.plugins.bibles.forms.bibleimportform.BGExtract.get_bibles_from_http')
407
408=== added file 'tests/interfaces/openlp_plugins/songusage/__init__.py'
409=== modified file 'tests/resources/presentationmanagersongs/Agnus Dei.sng'
410--- tests/resources/presentationmanagersongs/Agnus Dei.sng 2014-10-13 11:38:13 +0000
411+++ tests/resources/presentationmanagersongs/Agnus Dei.sng 2017-03-02 21:00:50 +0000
412@@ -1,7 +1,7 @@
413 <?xml version="1.0" encoding="UTF-8"?>
414 <song xmlns="creativelifestyles/song">
415 <attributes>
416-<title>Agnus Dei</title>
417+<Title>Agnus Dei</Title>
418 <author></author>
419 <copyright></copyright>
420 <ccli_number></ccli_number>

Subscribers

People subscribed via source and target branches

to all changes: