openlp-core team mailing list archive

[Tomas Groth <tomasgroth@xxxxxxxx>]

Review: Needs Fixing

See below...

Diff comments:

> === 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-18 16:17:43 +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 'openlp/core/utils/__init__.py'
> --- openlp/core/utils/__init__.py	2015-01-19 08:34:29 +0000
> +++ openlp/core/utils/__init__.py	2015-02-18 16:17:43 +0000
> @@ -29,6 +29,7 @@
>  import os
>  import platform
>  import re
> +import socket
>  import time
>  from shutil import which
>  from subprocess import Popen, PIPE
> @@ -394,26 +395,33 @@
>          req.add_header('User-Agent', user_agent)
>      if header:
>          req.add_header(header[0], header[1])
> -    page = None
>      log.debug('Downloading URL = %s' % url)
> -    retries = 1
> -    while True:
> +    retries = 0
> +    while retries <= CONNECTION_RETRIES:
> +        retries += 1
> +        time.sleep(0.1)
>          try:
>              page = urllib.request.urlopen(req, timeout=CONNECTION_TIMEOUT)
> -            log.debug('Downloaded URL = %s' % page.geturl())
> -        except (urllib.error.URLError, ConnectionError):
> -            if retries > CONNECTION_RETRIES:
> -                log.exception('The web page could not be downloaded')
> -                raise
> -            else:
> -                retries += 1
> -                time.sleep(0.1)
> -                continue
> -        break
> -    if not page:
> -        return None
> +            log.debug('Downloaded page {}'.format(page.geturl()))
> +        except urllib.error.URLError as err:
> +            log.exception('URLError on {}'.format(url))
> +            log.exception('URLError: {}'.format(err.reason))
> +            page = None
> +        except socket.timeout:
> +            log.exception('Socket timeout: {}'.format(url))
> +            page = None
> +        except ConnectionRefusedError:
> +            log.exception('ConnectionRefused: {}'.format(url))
> +            page = None
> +            break
> +        except ConnectionError:
> +            log.exception('Connection error: {}'.format(url))
> +            page = None

It looks like you don't re-raises the exceptions anymore like it did before. Either you need to add it again or remove the try/except from firsttimeform.py (and possibly other places that uses get_web_page),

>      if update_openlp:
>          Registry().get('application').process_events()
> +    if not page:
> +        log.exception('{} could not be downloaded'.format(url))
> +        return None
>      log.debug(page)
>      return page
>  
> 
> === 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-18 16:17:43 +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')
> 


-- 
https://code.launchpad.net/~alisonken1/openlp/bug-1422683/+merge/250162
Your team OpenLP Core is requested to review the proposed merge of lp:~alisonken1/openlp/bug-1422683 into lp:openlp.