OpenLP

Merge lp:~tomasgroth/openlp/25bugfixes2 into lp:openlp

  1. 25bugfixes2
  2. Merge into trunk
Proposed by Tomas Groth
Status: Merged
Approved by: Raoul Snyman
Approved revision: 2640
Merged at revision: 2639
Proposed branch: lp:~tomasgroth/openlp/25bugfixes2
Merge into: lp:openlp
Diff against target: 195 lines (+51/-19)
9 files modified
openlp/core/ui/firsttimewizard.py (+7/-4)
openlp/core/ui/thememanager.py (+2/-2)
openlp/plugins/bibles/lib/http.py (+1/-1)
openlp/plugins/bibles/lib/mediaitem.py (+3/-0)
openlp/plugins/bibles/lib/zefania.py (+2/-1)
openlp/plugins/songs/lib/__init__.py (+2/-1)
openlp/plugins/songs/lib/importers/wordsofworship.py (+6/-6)
tests/functional/__init__.py (+3/-3)
tests/functional/openlp_plugins/songs/test_lib.py (+25/-1)
To merge this branch: bzr merge lp:~tomasgroth/openlp/25bugfixes2
Reviewer Review Type Date Requested Status
Raoul Snyman Approve
Tim Bentley Approve
Review via email: mp+291465@code.launchpad.net

This proposal supersedes a proposal from 2016-03-29.

Description of the change

Fix traceback when searching for book that doesn't exists in second bible. Fixes bug 1553863.
Set progress bar steps to number of chapters in zefania import.
Use standard button text in the FTW, if possible. Fixes bug 1554554.
Translation fixes. Fixes bug 1545072
Fix song tag detection. Fixes bug 1549549.
Fix a method call with too many parentheses, which fixes getting bible books from crosswalk.

To post a comment you must log in.
Revision history for this message
Tomas Groth (tomasgroth) wrote : Posted in a previous version of this proposal

lp:~tomasgroth/openlp/25bugfixes2 (revision 2637)
[SUCCESS] https://ci.openlp.io/job/Branch-01-Pull/1345/
[SUCCESS] https://ci.openlp.io/job/Branch-02-Functional-Tests/1265/
[SUCCESS] https://ci.openlp.io/job/Branch-03-Interface-Tests/1204/
[SUCCESS] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1039/
[SUCCESS] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/630/
[SUCCESS] https://ci.openlp.io/job/Branch-05a-Code_Analysis/697/
[SUCCESS] https://ci.openlp.io/job/Branch-05b-Test_Coverage/565/

Revision history for this message
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal

All good, would just prefer except <SomeTypeOfExceptionHere> over a catch-all except statement.

review: Needs Information
Revision history for this message
Tomas Groth (tomasgroth) wrote :

Made the exception catch more specific, as requested.

lp:~tomasgroth/openlp/25bugfixes2 (revision 2640)
[SUCCESS] https://ci.openlp.io/job/Branch-01-Pull/1415/
[SUCCESS] https://ci.openlp.io/job/Branch-02-Functional-Tests/1333/
[SUCCESS] https://ci.openlp.io/job/Branch-03-Interface-Tests/1272/
[SUCCESS] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1085/
[SUCCESS] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/676/
[SUCCESS] https://ci.openlp.io/job/Branch-05a-Code_Analysis/743/
[SUCCESS] https://ci.openlp.io/job/Branch-05b-Test_Coverage/611/

Revision history for this message
Tim Bentley (trb143) :
review: Approve
Revision history for this message
Raoul Snyman (raoul-snyman) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'openlp/core/ui/firsttimewizard.py'
2--- openlp/core/ui/firsttimewizard.py 2015-12-31 22:46:06 +0000
3+++ openlp/core/ui/firsttimewizard.py 2016-04-10 20:39:44 +0000
4@@ -249,10 +249,11 @@
5 self.no_internet_text = translate('OpenLP.FirstTimeWizard',
6 'No Internet connection was found. The First Time Wizard needs an Internet '
7 'connection in order to be able to download sample songs, Bibles and themes.'
8- ' Click the Finish button now to start OpenLP with initial settings and '
9+ ' Click the %s button now to start OpenLP with initial settings and '
10 'no sample data.\n\nTo re-run the First Time Wizard and import this sample '
11 'data at a later time, check your Internet connection and re-run this '
12- 'wizard by selecting "Tools/Re-run First Time Wizard" from OpenLP.')
13+ 'wizard by selecting "Tools/Re-run First Time Wizard" from OpenLP.') % \
14+ clean_button_text(first_time_wizard.buttonText(QtWidgets.QWizard.FinishButton))
15 self.cancel_wizard_text = translate('OpenLP.FirstTimeWizard',
16 '\n\nTo cancel the First Time Wizard completely (and not start OpenLP), '
17 'click the %s button now.') % \
18@@ -272,5 +273,7 @@
19 self.progress_page.setSubTitle(translate('OpenLP.FirstTimeWizard', 'Please wait while resources are downloaded '
20 'and OpenLP is configured.'))
21 self.progress_label.setText(translate('OpenLP.FirstTimeWizard', 'Starting configuration process...'))
22- first_time_wizard.setButtonText(QtWidgets.QWizard.CustomButton1, translate('OpenLP.FirstTimeWizard', 'Finish'))
23- first_time_wizard.setButtonText(QtWidgets.QWizard.CustomButton2, translate('OpenLP.FirstTimeWizard', 'Cancel'))
24+ first_time_wizard.setButtonText(QtWidgets.QWizard.CustomButton1,
25+ clean_button_text(first_time_wizard.buttonText(QtWidgets.QWizard.FinishButton)))
26+ first_time_wizard.setButtonText(QtWidgets.QWizard.CustomButton2,
27+ clean_button_text(first_time_wizard.buttonText(QtWidgets.QWizard.CancelButton)))
28
29=== modified file 'openlp/core/ui/thememanager.py'
30--- openlp/core/ui/thememanager.py 2016-04-05 17:10:51 +0000
31+++ openlp/core/ui/thememanager.py 2016-04-10 20:39:44 +0000
32@@ -760,8 +760,8 @@
33 used_count = plugin.uses_theme(theme)
34 if used_count:
35 plugin_usage = "%s%s" % (plugin_usage, (translate('OpenLP.ThemeManager',
36- '%s time(s) by %s') %
37- (used_count, plugin.name)))
38+ '%(count)s time(s) by %(plugin)s') %
39+ {'count': used_count, 'plugin': plugin.name}))
40 plugin_usage = "%s\n" % plugin_usage
41 if plugin_usage:
42 critical_error_message_box(translate('OpenLP.ThemeManager', 'Unable to delete theme'),
43
44=== modified file 'openlp/plugins/bibles/lib/http.py'
45--- openlp/plugins/bibles/lib/http.py 2016-04-05 18:44:50 +0000
46+++ openlp/plugins/bibles/lib/http.py 2016-04-10 20:39:44 +0000
47@@ -504,7 +504,7 @@
48 soup = get_soup_for_bible_ref(chapter_url)
49 if not soup:
50 return None
51- content = soup.find_all(('h4', {'class': 'small-header'}))
52+ content = soup.find_all('h4', {'class': 'small-header'})
53 if not content:
54 log.error('No books found in the Crosswalk response.')
55 send_error_message('parse')
56
57=== modified file 'openlp/plugins/bibles/lib/mediaitem.py'
58--- openlp/plugins/bibles/lib/mediaitem.py 2016-04-03 19:44:09 +0000
59+++ openlp/plugins/bibles/lib/mediaitem.py 2016-04-10 20:39:44 +0000
60@@ -764,6 +764,9 @@
61 except IndexError:
62 log.exception('The second_search_results does not have as many verses as the search_results.')
63 break
64+ except TypeError:
65+ log.exception('The second_search_results does not have this book.')
66+ break
67 bible_text = '%s %d%s%d (%s, %s)' % (book, verse.chapter, verse_separator, verse.verse, version,
68 second_version)
69 else:
70
71=== modified file 'openlp/plugins/bibles/lib/zefania.py'
72--- openlp/plugins/bibles/lib/zefania.py 2015-12-31 22:46:06 +0000
73+++ openlp/plugins/bibles/lib/zefania.py 2016-04-10 20:39:44 +0000
74@@ -70,7 +70,8 @@
75 log.error('Importing books from "%s" failed' % self.filename)
76 return False
77 self.save_meta('language_id', language_id)
78- num_books = int(zefania_bible_tree.xpath("count(//BIBLEBOOK)"))
79+ num_books = int(zefania_bible_tree.xpath('count(//BIBLEBOOK)'))
80+ self.wizard.progress_bar.setMaximum(int(zefania_bible_tree.xpath('count(//CHAPTER)')))
81 # Strip tags we don't use - keep content
82 etree.strip_tags(zefania_bible_tree, ('STYLE', 'GRAM', 'NOTE', 'SUP', 'XREF'))
83 # Strip tags we don't use - remove content
84
85=== modified file 'openlp/plugins/songs/lib/__init__.py'
86--- openlp/plugins/songs/lib/__init__.py 2016-04-05 17:30:20 +0000
87+++ openlp/plugins/songs/lib/__init__.py 2016-04-10 20:39:44 +0000
88@@ -255,6 +255,7 @@
89 for num, translation in enumerate(VerseType.translated_names):
90 if verse_name == translation.lower():
91 return num
92+ return None
93
94 @staticmethod
95 def from_loose_input(verse_name, default=Other):
96@@ -270,7 +271,7 @@
97 if verse_index is None:
98 verse_index = VerseType.from_string(verse_name, default)
99 elif len(verse_name) == 1:
100- verse_index = VerseType.from_translated_tag(verse_name, default)
101+ verse_index = VerseType.from_translated_tag(verse_name, None)
102 if verse_index is None:
103 verse_index = VerseType.from_tag(verse_name, default)
104 else:
105
106=== modified file 'openlp/plugins/songs/lib/importers/wordsofworship.py'
107--- openlp/plugins/songs/lib/importers/wordsofworship.py 2015-12-31 22:46:06 +0000
108+++ openlp/plugins/songs/lib/importers/wordsofworship.py 2016-04-10 20:39:44 +0000
109@@ -107,9 +107,9 @@
110 song_data = open(source, 'rb')
111 if song_data.read(19).decode() != 'WoW File\nSong Words':
112 self.log_error(source,
113- str(translate('SongsPlugin.WordsofWorshipSongImport',
114- 'Invalid Words of Worship song file. Missing "%s" header.'
115- % 'WoW File\\nSong Words')))
116+ translate('SongsPlugin.WordsofWorshipSongImport',
117+ 'Invalid Words of Worship song file. Missing "%s" header.')
118+ % 'WoW File\\nSong Words')
119 continue
120 # Seek to byte which stores number of blocks in the song
121 song_data.seek(56)
122@@ -117,9 +117,9 @@
123 song_data.seek(66)
124 if song_data.read(16).decode() != 'CSongDoc::CBlock':
125 self.log_error(source,
126- str(translate('SongsPlugin.WordsofWorshipSongImport',
127- 'Invalid Words of Worship song file. Missing "%s" '
128- 'string.' % 'CSongDoc::CBlock')))
129+ translate('SongsPlugin.WordsofWorshipSongImport',
130+ 'Invalid Words of Worship song file. Missing "%s" string.')
131+ % 'CSongDoc::CBlock')
132 continue
133 # Seek to the beginning of the first block
134 song_data.seek(82)
135
136=== modified file 'tests/functional/__init__.py'
137--- tests/functional/__init__.py 2015-12-31 22:46:06 +0000
138+++ tests/functional/__init__.py 2016-04-10 20:39:44 +0000
139@@ -26,12 +26,12 @@
140 from PyQt5 import QtWidgets
141
142 if sys.version_info[1] >= 3:
143- from unittest.mock import ANY, MagicMock, patch, mock_open, call
144+ from unittest.mock import ANY, MagicMock, patch, mock_open, call, PropertyMock
145 else:
146- from mock import ANY, MagicMock, patch, mock_open, call
147+ from mock import ANY, MagicMock, patch, mock_open, call, PropertyMock
148
149 # Only one QApplication can be created. Use QtWidgets.QApplication.instance() when you need to "create" a QApplication.
150 application = QtWidgets.QApplication([])
151 application.setApplicationName('OpenLP')
152
153-__all__ = ['ANY', 'MagicMock', 'patch', 'mock_open', 'call', 'application']
154+__all__ = ['ANY', 'MagicMock', 'patch', 'mock_open', 'call', 'application', 'PropertyMock']
155
156=== modified file 'tests/functional/openlp_plugins/songs/test_lib.py'
157--- tests/functional/openlp_plugins/songs/test_lib.py 2015-12-31 22:46:06 +0000
158+++ tests/functional/openlp_plugins/songs/test_lib.py 2016-04-10 20:39:44 +0000
159@@ -26,7 +26,7 @@
160
161 from openlp.plugins.songs.lib import VerseType, clean_string, clean_title, strip_rtf
162 from openlp.plugins.songs.lib.songcompare import songs_probably_equal, _remove_typos, _op_length
163-from tests.functional import patch, MagicMock
164+from tests.functional import patch, MagicMock, PropertyMock
165
166
167 class TestLib(TestCase):
168@@ -477,3 +477,27 @@
169
170 # THEN: The result should be None
171 self.assertIsNone(result, 'The result should be None, but was "%s"' % result)
172+
173+ @patch('openlp.plugins.songs.lib.VerseType.translated_tags', new_callable=PropertyMock, return_value=['x'])
174+ def from_loose_input_with_invalid_input_test(self, mocked_translated_tags):
175+ """
176+ Test that the from_loose_input() method returns a sane default when passed an invalid tag and None as default.
177+ """
178+ # GIVEN: A mocked VerseType.translated_tags
179+ # WHEN: We run the from_loose_input() method with an invalid verse type, we get the specified default back
180+ result = VerseType.from_loose_input('m', None)
181+
182+ # THEN: The result should be None
183+ self.assertIsNone(result, 'The result should be None, but was "%s"' % result)
184+
185+ @patch('openlp.plugins.songs.lib.VerseType.translated_tags', new_callable=PropertyMock, return_value=['x'])
186+ def from_loose_input_with_valid_input_test(self, mocked_translated_tags):
187+ """
188+ Test that the from_loose_input() method returns valid output on valid input.
189+ """
190+ # GIVEN: A mocked VerseType.translated_tags
191+ # WHEN: We run the from_loose_input() method with a valid verse type, we get the expected VerseType back
192+ result = VerseType.from_loose_input('v')
193+
194+ # THEN: The result should be a Verse
195+ self.assertEqual(result, VerseType.Verse, 'The result should be a verse, but was "%s"' % result)