openlp-core team mailing list archive

[Ken Roberts <alisonken1@xxxxxxxxx>]

Ken Roberts has proposed merging lp:~alisonken1/openlp/bug-1422683 into lp:openlp.

Requested reviews:
  OpenLP Core (openlp-core)
Related bugs:
  Bug #1422683 in OpenLP: "The FTW fails when network is disconnected during download"
  https://bugs.launchpad.net/openlp/+bug/1422683

For more details, see:
https://code.launchpad.net/~alisonken1/openlp/bug-1422683/+merge/250074

Fix bug 1422683

lp:~alisonken1/openlp/bug-1422683 (revision 2502)
[SUCCESS] http://ci.openlp.org/job/Branch-01-Pull/948/
[SUCCESS] http://ci.openlp.org/job/Branch-02-Functional-Tests/874/
[SUCCESS] http://ci.openlp.org/job/Branch-03-Interface-Tests/819/
[SUCCESS] http://ci.openlp.org/job/Branch-04a-Windows_Functional_Tests/715/
[SUCCESS] http://ci.openlp.org/job/Branch-04b-Windows_Interface_Tests/314/
[FAILURE] http://ci.openlp.org/job/Branch-05a-Code_Analysis/451/

failure in code analysis is:
openlp/branch/openlp/core/common/historycombobox.py
not part of this merge
-- 
Your team OpenLP Core is requested to review the proposed merge of lp:~alisonken1/openlp/bug-1422683 into lp:openlp.

=== modified file 'openlp/core/ui/firsttimeform.py'
--- openlp/core/ui/firsttimeform.py	2015-02-17 18:50:10 +0000
+++ openlp/core/ui/firsttimeform.py	2015-02-17 21:37:24 +0000
@@ -25,6 +25,7 @@
 import hashlib
 import logging
 import os
+import socket
 import time
 import urllib.request
 import urllib.parse
@@ -403,8 +404,8 @@
         retries = 0
         while True:
             try:
+                filename = open(f_path, "wb")
                 url_file = urllib.request.urlopen(url, timeout=CONNECTION_TIMEOUT)
-                filename = open(f_path, "wb")
                 if sha256:
                     hasher = hashlib.sha256()
                 # Download until finished or canceled.
@@ -422,7 +423,7 @@
                     log.error('sha256 sums did not match for file: {}'.format(f_path))
                     os.remove(f_path)
                     return False
-            except urllib.error.URLError:
+            except (urllib.error.URLError, socket.timeout) as err:
                 trace_error_handler(log)
                 filename.close()
                 os.remove(f_path)
@@ -617,6 +618,7 @@
         songs_destination = os.path.join(gettempdir(), 'openlp')
         bibles_destination = AppLocation.get_section_data_path('bibles')
         themes_destination = AppLocation.get_section_data_path('themes')
+        missed_files = []
         # Download songs
         for i in range(self.songs_list_widget.count()):
             item = self.songs_list_widget.item(i)
@@ -626,7 +628,7 @@
                 self.previous_size = 0
                 destination = os.path.join(songs_destination, str(filename))
                 if not self.url_get_file('%s%s' % (self.songs_url, filename), destination, sha256):
-                    return False
+                    missed_files.append('Song: {}'.format(filename))
         # Download Bibles
         bibles_iterator = QtGui.QTreeWidgetItemIterator(self.bibles_tree_widget)
         while bibles_iterator.value():
@@ -637,7 +639,7 @@
                 self.previous_size = 0
                 if not self.url_get_file('%s%s' % (self.bibles_url, bible), os.path.join(bibles_destination, bible),
                                          sha256):
-                    return False
+                    missed_files.append('Bible: {}'.format(bible))
             bibles_iterator += 1
         # Download themes
         for i in range(self.themes_list_widget.count()):
@@ -648,7 +650,20 @@
                 self.previous_size = 0
                 if not self.url_get_file('%s%s' % (self.themes_url, theme), os.path.join(themes_destination, theme),
                                          sha256):
-                    return False
+                    missed_files.append('Theme: {}'.format(theme))
+        if missed_files:
+            file_list = ''
+            for entry in missed_files:
+                file_list += '{}<br \>'.format(entry)
+            msg = QtGui.QMessageBox()
+            msg.setIcon(QtGui.QMessageBox.Warning)
+            msg.setWindowTitle(translate('OpenLP.FirstTimeWizard', 'Network Error'))
+            msg.setText(translate('OpenLP.FirstTimeWizard', 'Unable to download some files'))
+            msg.setInformativeText(translate('OpenLP.FirstTimeWizard',
+                                             'The following files were not able to be '
+                                             'downloaded:<br \>{}'.format(file_list)))
+            msg.setStandardButtons(msg.Ok)
+            ans = msg.exec_()
         return True
 
     def _set_plugin_status(self, field, tag):

=== modified file 'tests/functional/openlp_core_ui/test_firsttimeform.py'
--- tests/functional/openlp_core_ui/test_firsttimeform.py	2015-01-19 08:34:29 +0000
+++ tests/functional/openlp_core_ui/test_firsttimeform.py	2015-02-17 21:37:24 +0000
@@ -23,6 +23,8 @@
 Package to test the openlp.core.ui.firsttimeform package.
 """
 import os
+import socket
+import tempfile
 import urllib
 from unittest import TestCase
 
@@ -70,6 +72,11 @@
         self.app.process_events = lambda: None
         Registry.create()
         Registry().register('application', self.app)
+        self.tempfile = os.path.join(tempfile.gettempdir(), 'testfile')
+
+    def tearDown(self):
+        if os.path.isfile(self.tempfile):
+            os.remove(self.tempfile)
 
     def initialise_test(self):
         """
@@ -229,3 +236,20 @@
         # THEN: the critical_error_message_box should have been called
         self.assertEquals(mocked_message_box.mock_calls[1][1][0], 'Network Error 407',
                           'first_time_form should have caught Network Error')
+
+    @patch('openlp.core.ui.firsttimeform.urllib.request.urlopen')
+    def socket_timeout_test(self, mocked_urlopen):
+        """
+        Test socket timeout gets caught
+        """
+        # GIVEN: Mocked urlopen to fake a network disconnect in the middle of a download
+        first_time_form = FirstTimeForm(None)
+        first_time_form.initialize(MagicMock())
+        mocked_urlopen.side_effect = socket.timeout()
+
+        # WHEN: Attempt to retrieve a file
+        first_time_form.url_get_file(url='http://localhost/test', f_path=self.tempfile)
+
+        # THEN: socket.timeout should have been caught
+        # NOTE: Test is if $tmpdir/tempfile is still there, then test fails since ftw deletes bad downloaded files
+        self.assertFalse(os.path.exists(self.tempfile), 'FTW url_get_file should have caught socket.timeout')