Various fixes and things:

- Update SongSelect importer to handle changes to the SongSelect site
- Fix PresentationManager importer to take weird XML into account
- Fix OpenLP importer to support author_types
- Fix OpenLP importer to support song numbers
- Fix opening the data folder in KDE where it was being misinterpreted as a SMB share
- Fix a problem with the media player no longer controlling the playlist
- Fix a problem where a timer would expire after the application had been torn down
...

bzr-revno: 2728

CHANGELOG.rst

@@ -0,0 +1,11 @@
+OpenLP 2.5.1
+============
+
+* Fixed a bug where the author type upgrade was being ignore because it was looking at the wrong table
+* Fixed a bug where the songs_songbooks table was not being created because the if expression was the wrong way round
+* Changed the songs_songbooks migration SQL slightly to take into account a bug that has (hopefully) been fixed
+* Sometimes the timer goes off as OpenLP is shutting down, and the application has already been deleted (reported via support system)
+* Fix opening the data folder (KDE thought the old way was an SMB share)
+* Fix a problem with the new QMediaPlayer not controlling the playlist anymore
+* Added importing of author types to the OpenLP 2 song importer
+* Refactored the merge script and gave it some options

openlp/core/__init__.py

@@ -319,7 +319,7 @@ class OpenLP(OpenLPMixin, QtWidgets.QApplication):
         return QtWidgets.QApplication.event(self, event)
 
 
-def parse_options(args):
+def parse_options(args=None):
     """
     Parse the command line arguments
 

openlp/core/ui/maindisplay.py

@@ -689,7 +689,7 @@ class AudioPlayer(OpenLPMixin, QtCore.QObject):
         """
         Skip forward to the next track in the list
         """
-        self.player.next()
+        self.playerlist.next()
 
     def go_to(self, index):
         """

openlp/core/ui/mainwindow.py

@@ -766,7 +766,7 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties):
         Use the Online manual in other cases. (Linux)
         """
         if is_macosx() or is_win():
-            QtGui.QDesktopServices.openUrl(QtCore.QUrl("file:///" + self.local_help_file))
+            QtGui.QDesktopServices.openUrl(QtCore.QUrl.fromLocalFile(self.local_help_file))
         else:
             import webbrowser
             webbrowser.open_new('http://manual.openlp.org/')
@@ -789,7 +789,7 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties):
         Open data folder
         """
         path = AppLocation.get_data_path()
-        QtGui.QDesktopServices.openUrl(QtCore.QUrl("file:///" + path))
+        QtGui.QDesktopServices.openUrl(QtCore.QUrl.fromLocalFile(path))
 
     def on_update_theme_images(self):
         """
@@ -1393,6 +1393,8 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties):
         if event.timerId() == self.timer_id:
             self.timer_id = 0
             self.load_progress_bar.hide()
+            # Sometimes the timer goes off as OpenLP is shutting down, and the application has already been deleted
+            if self.application:
                 self.application.process_events()
 
     def set_new_data_path(self, new_data_path):

openlp/plugins/songs/lib/importers/openlp.py

@@ -61,6 +61,12 @@ class OpenLPSongImport(SongImport):
         :param progress_dialog: The QProgressDialog used when importing songs from the FRW.
         """
 
+        class OldAuthorSong(BaseModel):
+            """
+            Maps to the authors_songs table
+            """
+            pass
+
         class OldAuthor(BaseModel):
             """
             Maps to the authors table
@@ -109,14 +115,19 @@ class OpenLPSongImport(SongImport):
         source_meta.reflect(engine)
         self.source_session = scoped_session(sessionmaker(bind=engine))
         # Run some checks to see which version of the database we have
-        if 'media_files' in list(source_meta.tables.keys()):
+        table_list = list(source_meta.tables.keys())
+        if 'media_files' in table_list:
             has_media_files = True
         else:
             has_media_files = False
-        if 'songs_songbooks' in list(source_meta.tables.keys()):
+        if 'songs_songbooks' in table_list:
             has_songs_books = True
         else:
             has_songs_books = False
+        if 'authors_songs' in table_list:
+            has_authors_songs = True
+        else:
+            has_authors_songs = False
         # Load up the tabls and map them out
         source_authors_table = source_meta.tables['authors']
         source_song_books_table = source_meta.tables['song_books']
@@ -139,6 +150,10 @@ class OpenLPSongImport(SongImport):
                 class_mapper(OldSongBookEntry)
             except UnmappedClassError:
                 mapper(OldSongBookEntry, source_songs_songbooks_table, properties={'songbook': relation(OldBook)})
+        if has_authors_songs and 'author_type' in source_authors_songs_table.c.values():
+            has_author_type = True
+        else:
+            has_author_type = False
         # Set up the songs relationships
         song_props = {
             'authors': relation(OldAuthor, backref='songs', secondary=source_authors_songs_table),
@@ -157,6 +172,8 @@ class OpenLPSongImport(SongImport):
             song_props['songbook_entries'] = relation(OldSongBookEntry, backref='song', cascade='all, delete-orphan')
         else:
             song_props['book'] = relation(OldBook, backref='songs')
+        if has_authors_songs:
+            song_props['authors_songs'] = relation(OldAuthorSong)
         # Map the rest of the tables
         try:
             class_mapper(OldAuthor)
@@ -174,6 +191,11 @@ class OpenLPSongImport(SongImport):
             class_mapper(OldTopic)
         except UnmappedClassError:
             mapper(OldTopic, source_topics_table)
+        if has_authors_songs:
+            try:
+                class_mapper(OldTopic)
+            except UnmappedClassError:
+                mapper(OldTopic, source_topics_table)
 
         source_songs = self.source_session.query(OldSong).all()
         if self.import_wizard:
@@ -205,8 +227,16 @@ class OpenLPSongImport(SongImport):
                     existing_author = Author.populate(
                         first_name=author.first_name,
                         last_name=author.last_name,
-                        display_name=author.display_name)
-                new_song.add_author(existing_author)
+                        display_name=author.display_name
+                    )
+                # If this is a new database, we need to import the author_type too
+                author_type = None
+                if has_author_type:
+                    for author_song in song.authors_songs:
+                        if author_song.author_id == author.id:
+                            author_type = author_song.author_type
+                            break
+                new_song.add_author(existing_author, author_type)
             # Find or create all the topics and add them to the new song object
             if song.topics:
                 for topic in song.topics:
@@ -221,10 +251,16 @@ class OpenLPSongImport(SongImport):
                     if not existing_book:
                         existing_book = Book.populate(name=entry.songbook.name, publisher=entry.songbook.publisher)
                     new_song.add_songbook_entry(existing_book, entry.entry)
-            elif song.book:
+            elif hasattr(song, 'book') and song.book:
                 existing_book = self.manager.get_object_filtered(Book, Book.name == song.book.name)
                 if not existing_book:
                     existing_book = Book.populate(name=song.book.name, publisher=song.book.publisher)
+                # Get the song_number from "songs" table "song_number" field. (This is db structure from 2.2.1)
+                # If there's a number, add it to the song, otherwise it will be "".
+                existing_number = song.song_number if hasattr(song, 'song_number') else ''
+                if existing_number:
+                    new_song.add_songbook_entry(existing_book, existing_number)
+                else:
                     new_song.add_songbook_entry(existing_book, '')
             # Find or create all the media files and add them to the new song object
             if has_media_files and song.media_files:

openlp/plugins/songs/lib/importers/presentationmanager.py

@@ -23,7 +23,6 @@
 The :mod:`presentationmanager` module provides the functionality for importing
 Presentationmanager song files into the current database.
 """
-
 import os
 import re
 
@@ -65,15 +64,34 @@ class PresentationManagerImport(SongImport):
                                              'File is not in XML-format, which is the only format supported.'))
                     continue
             root = objectify.fromstring(etree.tostring(tree))
-            self.process_song(root)
+            self.process_song(root, file_path)
 
-    def process_song(self, root):
+    def _get_attr(self, elem, name):
+        """
+        Due to PresentationManager's habit of sometimes capitilising the first letter of an element, we have to do
+        some gymnastics.
+        """
+        if hasattr(elem, name):
+            return str(getattr(elem, name))
+        name = name[0].upper() + name[1:]
+        if hasattr(elem, name):
+            return str(getattr(elem, name))
+        else:
+            return ''
+
+    def process_song(self, root, file_path):
         self.set_defaults()
-        self.title = str(root.attributes.title)
-        self.add_author(str(root.attributes.author))
-        self.copyright = str(root.attributes.copyright)
-        self.ccli_number = str(root.attributes.ccli_number)
-        self.comments = str(root.attributes.comments)
+        attrs = None
+        if hasattr(root, 'attributes'):
+            attrs = root.attributes
+        elif hasattr(root, 'Attributes'):
+            attrs = root.Attributes
+        if attrs is not None:
+            self.title = self._get_attr(root.attributes, 'title')
+            self.add_author(self._get_attr(root.attributes, 'author'))
+            self.copyright = self._get_attr(root.attributes, 'copyright')
+            self.ccli_number = self._get_attr(root.attributes, 'ccli_number')
+            self.comments = str(root.attributes.comments) if hasattr(root.attributes, 'comments') else None
         verse_order_list = []
         verse_count = {}
         duplicates = []
@@ -105,4 +123,4 @@ class PresentationManagerImport(SongImport):
 
         self.verse_order_list = verse_order_list
         if not self.finish():
-            self.log_error(self.import_source)
+            self.log_error(os.path.basename(file_path))

openlp/plugins/songs/lib/songselect.py

@@ -47,7 +47,7 @@ USER_AGENTS = [
 BASE_URL = 'https://songselect.ccli.com'
 LOGIN_PAGE = 'https://profile.ccli.com/account/signin?appContext=SongSelect&returnUrl='\
     'https%3a%2f%2fsongselect.ccli.com%2f'
-LOGIN_URL = 'https://profile.ccli.com/'
+LOGIN_URL = 'https://profile.ccli.com'
 LOGOUT_URL = BASE_URL + '/account/logout'
 SEARCH_URL = BASE_URL + '/search/results'
 
@@ -97,14 +97,27 @@ class SongSelectImport(object):
             'password': password,
             'RememberMe': 'false'
         })
+        login_form = login_page.find('form')
+        if login_form:
+            login_url = login_form.attrs['action']
+        else:
+            login_url = '/Account/SignIn'
+        if not login_url.startswith('http'):
+            if login_url[0] != '/':
+                login_url = '/' + login_url
+            login_url = LOGIN_URL + login_url
         try:
-            posted_page = BeautifulSoup(self.opener.open(LOGIN_URL, data.encode('utf-8')).read(), 'lxml')
+            posted_page = BeautifulSoup(self.opener.open(login_url, data.encode('utf-8')).read(), 'lxml')
         except (TypeError, URLError) as error:
             log.exception('Could not login to SongSelect, {error}'.format(error=error))
             return False
         if callback:
             callback()
-        return posted_page.find('input', id='SearchText') is not None
+        if posted_page.find('input', id='SearchText') is not None:
+            return True
+        else:
+            log.debug(posted_page)
+            return False
 
     def logout(self):
         """

openlp/plugins/songs/lib/upgrade.py

@@ -32,7 +32,7 @@ from openlp.core.common.db import drop_columns
 from openlp.core.lib.db import get_upgrade_op
 
 log = logging.getLogger(__name__)
-__version__ = 5
+__version__ = 6
 
 
 # TODO: When removing an upgrade path the ftw-data needs updating to the minimum supported version
@@ -52,7 +52,6 @@ def upgrade_1(session, metadata):
     :param metadata:
     """
     op = get_upgrade_op(session)
-    songs_table = Table('songs', metadata, autoload=True)
     if 'media_files_songs' in [t.name for t in metadata.tables.values()]:
         op.drop_table('media_files_songs')
         op.add_column('media_files', Column('song_id', types.Integer(), server_default=null()))
@@ -102,21 +101,8 @@ def upgrade_4(session, metadata):
 
     This upgrade adds a column for author type to the authors_songs table
     """
-    # Since SQLite doesn't support changing the primary key of a table, we need to recreate the table
-    # and copy the old values
-    op = get_upgrade_op(session)
-    songs_table = Table('songs', metadata)
-    if 'author_type' not in [col.name for col in songs_table.c.values()]:
-        op.create_table('authors_songs_tmp',
-                        Column('author_id', types.Integer(), ForeignKey('authors.id'), primary_key=True),
-                        Column('song_id', types.Integer(), ForeignKey('songs.id'), primary_key=True),
-                        Column('author_type', types.Unicode(255), primary_key=True,
-                               nullable=False, server_default=text('""')))
-        op.execute('INSERT INTO authors_songs_tmp SELECT author_id, song_id, "" FROM authors_songs')
-        op.drop_table('authors_songs')
-        op.rename_table('authors_songs_tmp', 'authors_songs')
-    else:
-        log.warning('Skipping upgrade_4 step of upgrading the song db')
+    # This is now empty due to a bug in the upgrade
+    pass
 
 
 def upgrade_5(session, metadata):
@@ -125,21 +111,45 @@ def upgrade_5(session, metadata):
 
     This upgrade adds support for multiple songbooks
     """
-    op = get_upgrade_op(session)
-    songs_table = Table('songs', metadata)
-    if 'song_book_id' in [col.name for col in songs_table.c.values()]:
-        log.warning('Skipping upgrade_5 step of upgrading the song db')
-        return
+    # This is now empty due to a bug in the upgrade
+    pass
 
+
+def upgrade_6(session, metadata):
+    """
+    Version 6 upgrade
+
+    This version corrects the errors in upgrades 4 and 5
+    """
+    op = get_upgrade_op(session)
+    # Move upgrade 4 to here and correct it (authors_songs table, not songs table)
+    authors_songs = Table('authors_songs', metadata, autoload=True)
+    if 'author_type' not in [col.name for col in authors_songs.c.values()]:
+        # Since SQLite doesn't support changing the primary key of a table, we need to recreate the table
+        # and copy the old values
+        op.create_table(
+            'authors_songs_tmp',
+            Column('author_id', types.Integer(), ForeignKey('authors.id'), primary_key=True),
+            Column('song_id', types.Integer(), ForeignKey('songs.id'), primary_key=True),
+            Column('author_type', types.Unicode(255), primary_key=True,
+                   nullable=False, server_default=text('""'))
+        )
+        op.execute('INSERT INTO authors_songs_tmp SELECT author_id, song_id, "" FROM authors_songs')
+        op.drop_table('authors_songs')
+        op.rename_table('authors_songs_tmp', 'authors_songs')
+    # Move upgrade 5 here to correct it
+    if 'songs_songbooks' not in [t.name for t in metadata.tables.values()]:
         # Create the mapping table (songs <-> songbooks)
-    op.create_table('songs_songbooks',
+        op.create_table(
+            'songs_songbooks',
             Column('songbook_id', types.Integer(), ForeignKey('song_books.id'), primary_key=True),
             Column('song_id', types.Integer(), ForeignKey('songs.id'), primary_key=True),
-                    Column('entry', types.Unicode(255), primary_key=True, nullable=False))
+            Column('entry', types.Unicode(255), primary_key=True, nullable=False)
+        )
 
         # Migrate old data
         op.execute('INSERT INTO songs_songbooks SELECT song_book_id, id, song_number FROM songs\
-                WHERE song_book_id IS NOT NULL AND song_number IS NOT NULL')
+                    WHERE song_book_id IS NOT NULL AND song_number IS NOT NULL AND song_book_id <> 0')
 
         # Drop old columns
         if metadata.bind.url.get_dialect().name == 'sqlite':
@@ -148,3 +158,5 @@ def upgrade_5(session, metadata):
             op.drop_constraint('songs_ibfk_1', 'songs', 'foreignkey')
             op.drop_column('songs', 'song_book_id')
             op.drop_column('songs', 'song_number')
+    # Finally, clean up our mess in people's databases
+    op.execute('DELETE FROM songs_songbooks WHERE songbook_id = 0')

scripts/lp-merge.py

@@ -51,59 +51,72 @@ will print the detected bugs + author for easy copying into the GUI.
 """
 import subprocess
 import re
-import sys
 import os
-import urllib.request
+from urllib.request import urlopen
+from urllib.error import HTTPError
+from argparse import ArgumentParser
 from bs4 import BeautifulSoup
 
-# Check that the argument count is correct
-if len(sys.argv) != 2:
-    print('\n\tUsage: ' + sys.argv[0] + ' <url to approved merge request>\n')
-    exit()
 
-url = sys.argv[1]
-pattern = re.compile('.+?/\+merge/\d+')
-match = pattern.match(url)
+def parse_args():
+    """
+    Parse command line arguments and return them
+    """
+    parser = ArgumentParser(prog='lp-merge', description='A helper script to merge proposals on Launchpad')
+    parser.add_argument('url', help='URL to the merge proposal')
+    parser.add_argument('-y', '--yes-to-all', action='store_true', help='Presume yes for all queries')
+    parser.add_argument('-q', '--use-qcommit', action='store_true', help='Use qcommit for committing')
+    return parser.parse_args()
 
-# Check that the given argument is an url in the right format
+
+def is_merge_url_valid(url):
+    """
+    Determine if the merge URL is valid
+    """
+    match = re.match(r'.+?/\+merge/\d+', url)
     if not url.startswith('https://code.launchpad.net/~') or match is None:
         print('The url is not valid! It should look like this:\n    '
-          'https://code.launchpad.net/~tomasgroth/openlp/doc22update4/+merge/271874')
+              'https://code.launchpad.net/~myusername/openlp/mybranch/+merge/271874')
+        return False
+    return True
 
-page = urllib.request.urlopen(url)
+
+def get_merge_info(url):
+    """
+    Get all the merge information and return it as a dictionary
+    """
+    merge_info = {}
+    print('Fetching merge information...')
+    # Try to load the page
+    try:
+        page = urlopen(url)
+    except HTTPError:
+        print('Unable to load merge URL: {}'.format(url))
+        return None
     soup = BeautifulSoup(page.read(), 'lxml')
-
     # Find this span tag that contains the branch url
     # <span class="branch-url">
     span_branch_url = soup.find('span', class_='branch-url')
-branch_url = span_branch_url.contents[0]
-
+    if not span_branch_url:
+        print('Unable to find merge details on URL: {}'.format(url))
+        return None
+    merge_info['branch_url'] = span_branch_url.contents[0]
     # Find this tag that describes the branch. We'll use that for commit message
     # <meta name="description" content="...">
     meta = soup.find('meta', attrs={"name": "description"})
-
-commit_message = meta.get('content')
-
+    merge_info['commit_message'] = meta.get('content')
     # Find all tr-tags with this class. Makes it possible to get bug numbers.
     # <tr class="bug-branch-summary"
     bug_rows = soup.find_all('tr', class_='bug-branch-summary')
-bugs = []
+    merge_info['bugs'] = []
     for row in bug_rows:
         id_attr = row.get('id')
-    bugs.append(id_attr[8:])
-
+        merge_info['bugs'].append(id_attr[8:])
     # Find target branch name using the tag below
     # <div class="context-publication"><h1>Merge ... into...
     div_branches = soup.find('div', class_='context-publication')
     branches = div_branches.h1.contents[0]
-target_branch = '+branch/' + branches[(branches.find(' into lp:') + 9):]
-
-# Check that we are in the right branch
-bzr_info_output = subprocess.check_output(['bzr', 'info'])
-if target_branch not in bzr_info_output.decode():
-    print('ERROR: It seems you are not in the right folder...')
-    exit()
-
+    merge_info['target_branch'] = '+branch/' + branches[(branches.find(' into lp:') + 9):]
     # Find the authors email address. It is hidden in a javascript line like this:
     # conf = {"status_value": "Needs review", "source_revid": "tomasgroth@yahoo.dk-20160921204550-gxduegmcmty9rljf",
     #         "user_can_edit_status": false, ...
@@ -112,40 +125,99 @@ content = script_tag.contents[0]
     start_pos = content.find('source_revid') + 16
     pattern = re.compile('.*\w-\d\d\d\d\d+')
     match = pattern.match(content[start_pos:])
-author_email = match.group()[:-15]
+    merge_info['author_email'] = match.group()[:-15]
+    # Launchpad doesn't supply the author's true name, so we'll just grab whatever they use for display on LP
+    a_person = soup.find('div', id='registration').find('a', 'person')
+    merge_info['author_name'] = a_person.contents[0]
+    return merge_info
 
+
+def do_merge(merge_info, yes_to_all=False):
+    """
+    Do the merge
+    """
+    # Check that we are in the right branch
+    bzr_info_output = subprocess.check_output(['bzr', 'info'])
+    if merge_info['target_branch'] not in bzr_info_output.decode():
+        print('ERROR: It seems you are not in the right folder...')
+        return False
     # Merge the branch
-do_merge = input('Merge ' + branch_url + ' into local branch? (y/N/q): ').lower()
-if do_merge == 'y':
-    subprocess.call(['bzr', 'merge', branch_url])
-elif do_merge == 'q':
-    exit()
+    if yes_to_all:
+        can_merge = True
+    else:
+        user_choice = input('Merge ' + merge_info['branch_url'] + ' into local branch? (y/N/q): ').lower()
+        if user_choice == 'q':
+            return False
+        can_merge = user_choice == 'y'
+    if can_merge:
+        print('Merging...')
+        subprocess.call(['bzr', 'merge', merge_info['branch_url']])
+    return True
 
-# Create commit command
-commit_command = ['bzr', 'commit']
 
-for bug in bugs:
-    commit_command.append('--fixes')
-    commit_command.append('lp:' + bug)
-
-commit_command.append('-m')
-commit_command.append(commit_message)
-
-commit_command.append('--author')
-commit_command.append('"' + author_email + '"')
-
-print('About to run the bzr command below:\n')
-print(' '.join(commit_command))
-do_commit = input('Run the command (y), use qcommit (qcommit) or cancel (C): ').lower()
-
-if do_commit == 'y':
-    subprocess.call(commit_command)
-elif do_commit == 'qcommit':
+def qcommit(commit_message, author_name, author_email, bugs=[]):
+    """
+    Use qcommit to make the commit
+    """
     # Setup QT workaround to make qbzr look right on my box
     my_env = os.environ.copy()
     my_env['QT_GRAPHICSSYSTEM'] = 'native'
     # Print stuff that kan be copy/pasted into qbzr GUI
-    print('These bugs can be copy/pasted in: lp:' + ' lp:'.join(bugs))
-    print('The authors email is: ' + author_email)
+    if bugs:
+        print('These bugs can be copy/pasted in: ' + ' '.join(['lp:{}'.format(bug) for bug in bugs]))
+    print('The authors email is: {} <{}>'.format(author_name, author_email))
     # Run qcommit
     subprocess.call(['bzr', 'qcommit', '-m', commit_message], env=my_env)
+
+
+def do_commit(merge_info, yes_to_all=False, use_qcommit=False):
+    """
+    Actually do the commit
+    """
+    if use_qcommit:
+        qcommit(merge_info['commit_message'], merge_info['author_name'], merge_info['author_email'],
+                merge_info.get('bugs'))
+        return True
+    # Create commit command
+    commit_command = ['bzr', 'commit']
+    if 'bugs' in merge_info:
+        commit_command.extend(['--fixes=lp:{}'.format(bug) for bug in merge_info['bugs']])
+    commit_command.extend(['-m', merge_info['commit_message'],
+                           '--author', '{author_name} <{author_email}>'.format(**merge_info)])
+    if yes_to_all:
+        can_commit = True
+    else:
+        print('About to run the bzr command below:\n')
+        print(' '.join(commit_command))
+        user_choice = input('Run the command (y), use qcommit (q) or cancel (C): ').lower()
+        if user_choice == 'q':
+            qcommit(merge_info['commit_message'], merge_info['author_name'], merge_info['author_email'],
+                    merge_info.get('bugs'))
+            return True
+        can_commit = user_choice == 'y'
+    if can_commit:
+        print('Committing...')
+        subprocess.call(commit_command)
+        return True
+    else:
+        return False
+
+
+def main():
+    """
+    Run the script
+    """
+    args = parse_args()
+    if not is_merge_url_valid(args.url):
+        exit(1)
+    merge_info = get_merge_info(args.url)
+    if not merge_info:
+        exit(2)
+    if not do_merge(merge_info, args.yes_to_all):
+        exit(3)
+    if not do_commit(merge_info, args.yes_to_all, args.use_qcommit):
+        exit(4)
+
+
+if __name__ == '__main__':
+    main()

tests/functional/openlp_core/test_init.py

@@ -21,13 +21,13 @@
 ###############################################################################
 
 import sys
-from unittest import TestCase
+from unittest import TestCase, skip
+from unittest.mock import MagicMock, patch
 
-from openlp.core import parse_options
-from tests.helpers.testmixin import TestMixin
+from openlp.core import OpenLP, parse_options
 
 
-class TestInitFunctions(TestMixin, TestCase):
+class TestInitFunctions(TestCase):
 
     def test_parse_options_basic(self):
         """
@@ -116,8 +116,7 @@ class TestInitFunctions(TestMixin, TestCase):
 
     def test_parse_options_file_and_debug(self):
         """
-        Test the parse options process works with a file
-
+        Test the parse options process works with a file and the debug log level
         """
         # GIVEN: a a set of system arguments.
         sys.argv[1:] = ['-l debug', 'dummy_temp']
@@ -130,3 +129,30 @@ class TestInitFunctions(TestMixin, TestCase):
         self.assertFalse(args.portable, 'The portable flag should be set to false')
         self.assertEquals(args.style, None, 'There are no style flags to be processed')
         self.assertEquals(args.rargs, 'dummy_temp', 'The service file should not be blank')
+
+
+class TestOpenLP(TestCase):
+    """
+    Test the OpenLP app class
+    """
+    @skip('Figure out why this is causing a segfault')
+    @patch('openlp.core.QtWidgets.QApplication.exec')
+    def test_exec(self, mocked_exec):
+        """
+        Test the exec method
+        """
+        # GIVEN: An app
+        app = OpenLP([])
+        app.shared_memory = MagicMock()
+        mocked_exec.return_value = False
+
+        # WHEN: exec() is called
+        result = app.exec()
+
+        # THEN: The right things should be called
+        assert app.is_event_loop_active is True
+        mocked_exec.assert_called_once_with()
+        app.shared_memory.detach.assert_called_once_with()
+        assert result is False
+
+        del app

tests/functional/openlp_plugins/remotes/test_router.py

@@ -25,11 +25,11 @@ This module contains tests for the lib submodule of the Remotes plugin.
 import os
 import urllib.request
 from unittest import TestCase
+from unittest.mock import MagicMock, patch, mock_open
 
 from openlp.core.common import Settings, Registry
 from openlp.core.ui import ServiceManager
 from openlp.plugins.remotes.lib.httpserver import HttpRouter
-from tests.functional import MagicMock, patch, mock_open
 from tests.helpers.testmixin import TestMixin
 
 __default_settings__ = {
@@ -336,11 +336,9 @@ class TestRouter(TestCase, TestMixin):
         with patch.object(self.service_manager, 'setup_ui'), \
                 patch.object(self.router, 'do_json_header'):
             self.service_manager.bootstrap_initialise()
-            self.app.processEvents()
 
             # WHEN: Remote next is received
             self.router.service(action='next')
-            self.app.processEvents()
 
             # THEN: service_manager.next_item() should have been called
             self.assertTrue(mocked_next_item.called, 'next_item() should have been called in service_manager')
@@ -357,11 +355,9 @@ class TestRouter(TestCase, TestMixin):
         with patch.object(self.service_manager, 'setup_ui'), \
                 patch.object(self.router, 'do_json_header'):
             self.service_manager.bootstrap_initialise()
-            self.app.processEvents()
 
             # WHEN: Remote next is received
             self.router.service(action='previous')
-            self.app.processEvents()
 
             # THEN: service_manager.next_item() should have been called
             self.assertTrue(mocked_previous_item.called, 'previous_item() should have been called in service_manager')

tests/functional/openlp_plugins/songs/test_songselect.py

@@ -25,6 +25,7 @@ This module contains tests for the CCLI SongSelect importer.
 """
 import os
 from unittest import TestCase
+from unittest.mock import MagicMock, patch, call
 from urllib.error import URLError
 
 from PyQt5 import QtWidgets
@@ -32,9 +33,8 @@ from PyQt5 import QtWidgets
 from openlp.core import Registry
 from openlp.plugins.songs.forms.songselectform import SongSelectForm, SearchWorker
 from openlp.plugins.songs.lib import Song
-from openlp.plugins.songs.lib.songselect import SongSelectImport, LOGOUT_URL, BASE_URL
+from openlp.plugins.songs.lib.songselect import SongSelectImport, LOGIN_PAGE, LOGOUT_URL, BASE_URL
 
-from tests.functional import MagicMock, patch, call
 from tests.helpers.songfileimport import SongImportTestHelper
 from tests.helpers.testmixin import TestMixin
 
@@ -72,7 +72,9 @@ class TestSongSelectImport(TestCase, TestMixin):
         mocked_build_opener.return_value = mocked_opener
         mocked_login_page = MagicMock()
         mocked_login_page.find.side_effect = [{'value': 'blah'}, None]
-        MockedBeautifulSoup.return_value = mocked_login_page
+        mocked_posted_page = MagicMock()
+        mocked_posted_page.find.return_value = None
+        MockedBeautifulSoup.side_effect = [mocked_login_page, mocked_posted_page]
         mock_callback = MagicMock()
         importer = SongSelectImport(None)
 
@@ -82,6 +84,7 @@ class TestSongSelectImport(TestCase, TestMixin):
         # THEN: callback was called 3 times, open was called twice, find was called twice, and False was returned
         self.assertEqual(3, mock_callback.call_count, 'callback should have been called 3 times')
         self.assertEqual(2, mocked_login_page.find.call_count, 'find should have been called twice')
+        self.assertEqual(1, mocked_posted_page.find.call_count, 'find should have been called once')
         self.assertEqual(2, mocked_opener.open.call_count, 'opener should have been called twice')
         self.assertFalse(result, 'The login method should have returned False')
 
@@ -112,8 +115,10 @@ class TestSongSelectImport(TestCase, TestMixin):
         mocked_opener = MagicMock()
         mocked_build_opener.return_value = mocked_opener
         mocked_login_page = MagicMock()
-        mocked_login_page.find.side_effect = [{'value': 'blah'}, MagicMock()]
-        MockedBeautifulSoup.return_value = mocked_login_page
+        mocked_login_page.find.side_effect = [{'value': 'blah'}, None]
+        mocked_posted_page = MagicMock()
+        mocked_posted_page.find.return_value = MagicMock()
+        MockedBeautifulSoup.side_effect = [mocked_login_page, mocked_posted_page]
         mock_callback = MagicMock()
         importer = SongSelectImport(None)
 
@@ -122,10 +127,40 @@ class TestSongSelectImport(TestCase, TestMixin):
 
         # THEN: callback was called 3 times, open was called twice, find was called twice, and True was returned
         self.assertEqual(3, mock_callback.call_count, 'callback should have been called 3 times')
-        self.assertEqual(2, mocked_login_page.find.call_count, 'find should have been called twice')
+        self.assertEqual(2, mocked_login_page.find.call_count, 'find should have been called twice on the login page')
+        self.assertEqual(1, mocked_posted_page.find.call_count, 'find should have been called once on the posted page')
         self.assertEqual(2, mocked_opener.open.call_count, 'opener should have been called twice')
         self.assertTrue(result, 'The login method should have returned True')
 
+    @patch('openlp.plugins.songs.lib.songselect.build_opener')
+    @patch('openlp.plugins.songs.lib.songselect.BeautifulSoup')
+    def login_url_from_form_test(self, MockedBeautifulSoup, mocked_build_opener):
+        """
+        Test that the login URL is from the form
+        """
+        # GIVEN: A bunch of mocked out stuff and an importer object
+        mocked_opener = MagicMock()
+        mocked_build_opener.return_value = mocked_opener
+        mocked_form = MagicMock()
+        mocked_form.attrs = {'action': 'do/login'}
+        mocked_login_page = MagicMock()
+        mocked_login_page.find.side_effect = [{'value': 'blah'}, mocked_form]
+        mocked_posted_page = MagicMock()
+        mocked_posted_page.find.return_value = MagicMock()
+        MockedBeautifulSoup.side_effect = [mocked_login_page, mocked_posted_page]
+        mock_callback = MagicMock()
+        importer = SongSelectImport(None)
+
+        # WHEN: The login method is called after being rigged to fail
+        result = importer.login('username', 'password', mock_callback)
+
+        # THEN: callback was called 3 times, open was called twice, find was called twice, and True was returned
+        self.assertEqual(3, mock_callback.call_count, 'callback should have been called 3 times')
+        self.assertEqual(2, mocked_login_page.find.call_count, 'find should have been called twice on the login page')
+        self.assertEqual(1, mocked_posted_page.find.call_count, 'find should have been called once on the posted page')
+        self.assertEqual('https://profile.ccli.com/do/login', mocked_opener.open.call_args_list[1][0][0])
+        self.assertTrue(result, 'The login method should have returned True')
+
     @patch('openlp.plugins.songs.lib.songselect.build_opener')
     def test_logout(self, mocked_build_opener):
         """

tests/interfaces/openlp_plugins/bibles/forms/test_bibleimportform.py

@@ -22,17 +22,18 @@
 """
 Package to test the openlp.plugins.bibles.forms.bibleimportform package.
 """
-from unittest import TestCase
+from unittest import TestCase, skip
+from unittest.mock import MagicMock, patch
 
 from PyQt5 import QtWidgets
 
 from openlp.core.common import Registry
-import openlp.plugins.bibles.forms.bibleimportform as bibleimportform
+from openlp.plugins.bibles.forms.bibleimportform import BibleImportForm, PYSWORD_AVAILABLE
 
 from tests.helpers.testmixin import TestMixin
-from tests.functional import MagicMock, patch
 
 
+@skip('One of the QFormLayouts in the BibleImportForm is causing a segfault')
 class TestBibleImportForm(TestCase, TestMixin):
     """
     Test the BibleImportForm class
@@ -46,8 +47,9 @@ class TestBibleImportForm(TestCase, TestMixin):
         self.setup_application()
         self.main_window = QtWidgets.QMainWindow()
         Registry().register('main_window', self.main_window)
-        bibleimportform.PYSWORD_AVAILABLE = False
-        self.form = bibleimportform.BibleImportForm(self.main_window, MagicMock(), MagicMock())
+        PYSWORD_AVAILABLE = False
+        self.mocked_manager = MagicMock()
+        self.form = BibleImportForm(self.main_window, self.mocked_manager, MagicMock())
 
     def tearDown(self):
         """

tests/interfaces/openlp_plugins/songs/forms/test_songmaintenanceform.py

@@ -0,0 +1,292 @@
+# -*- coding: utf-8 -*-
+# vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4
+
+###############################################################################
+# OpenLP - Open Source Lyrics Projection                                      #
+# --------------------------------------------------------------------------- #
+# Copyright (c) 2008-2017 OpenLP Developers                                   #
+# --------------------------------------------------------------------------- #
+# This program is free software; you can redistribute it and/or modify it     #
+# under the terms of the GNU General Public License as published by the Free  #
+# Software Foundation; version 2 of the License.                              #
+#                                                                             #
+# This program is distributed in the hope that it will be useful, but WITHOUT #
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or       #
+# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for    #
+# more details.                                                               #
+#                                                                             #
+# You should have received a copy of the GNU General Public License along     #
+# with this program; if not, write to the Free Software Foundation, Inc., 59  #
+# Temple Place, Suite 330, Boston, MA 02111-1307 USA                          #
+###############################################################################
+"""
+Package to test the openlp.plugins.songs.forms.songmaintenanceform package.
+"""
+from unittest import TestCase
+from unittest.mock import MagicMock, patch, call
+
+from PyQt5 import QtCore, QtTest, QtWidgets
+
+from openlp.core.common import Registry, UiStrings
+from openlp.plugins.songs.forms.songmaintenanceform import SongMaintenanceForm
+
+from tests.helpers.testmixin import TestMixin
+
+
+class TestSongMaintenanceForm(TestCase, TestMixin):
+    """
+    Test the SongMaintenanceForm class
+    """
+
+    def setUp(self):
+        """
+        Create the UI
+        """
+        Registry.create()
+        self.setup_application()
+        self.main_window = QtWidgets.QMainWindow()
+        Registry().register('main_window', self.main_window)
+        self.mocked_manager = MagicMock()
+        self.form = SongMaintenanceForm(self.mocked_manager)
+
+    def tearDown(self):
+        """
+        Delete all the C++ objects at the end so that we don't have a segfault
+        """
+        del self.form
+        del self.main_window
+
+    def test_constructor(self):
+        """
+        Test that a SongMaintenanceForm is created successfully
+        """
+        # GIVEN: A SongMaintenanceForm
+        # WHEN: The form is created
+        # THEN: It should have some of the right components
+        assert self.form is not None
+        assert self.form.manager is self.mocked_manager
+
+    @patch.object(QtWidgets.QDialog, 'exec')
+    def test_exect(self, mocked_exec):
+        """
+        Test the song maintenance form being executed
+        """
+        # GIVEN: A song maintenance form
+        mocked_exec.return_value = True
+
+        # WHEN: The song mainetnance form is executed
+        with patch.object(self.form, 'type_list_widget') as mocked_type_list_widget, \
+                patch.object(self.form, 'reset_authors') as mocked_reset_authors, \
+                patch.object(self.form, 'reset_topics') as mocked_reset_topics, \
+                patch.object(self.form, 'reset_song_books') as mocked_reset_song_books:
+            result = self.form.exec(from_song_edit=True)
+
+        # THEN: The correct methods should have been called
+        assert self.form.from_song_edit is True
+        mocked_type_list_widget.setCurrentRow.assert_called_once_with(0)
+        mocked_reset_authors.assert_called_once_with()
+        mocked_reset_topics.assert_called_once_with()
+        mocked_reset_song_books.assert_called_once_with()
+        mocked_type_list_widget.setFocus.assert_called_once_with()
+        mocked_exec.assert_called_once_with(self.form)
+
+    def test_get_current_item_id_no_item(self):
+        """
+        Test _get_current_item_id() when there's no item
+        """
+        # GIVEN: A song maintenance form without a selected item
+        mocked_list_widget = MagicMock()
+        mocked_list_widget.currentItem.return_value = None
+
+        # WHEN: _get_current_item_id() is called
+        result = self.form._get_current_item_id(mocked_list_widget)
+
+        # THEN: The result should be -1
+        mocked_list_widget.currentItem.assert_called_once_with()
+        assert result == -1
+
+    def test_get_current_item_id(self):
+        """
+        Test _get_current_item_id() when there's a valid item
+        """
+        # GIVEN: A song maintenance form with a selected item
+        mocked_item = MagicMock()
+        mocked_item.data.return_value = 7
+        mocked_list_widget = MagicMock()
+        mocked_list_widget.currentItem.return_value = mocked_item
+
+        # WHEN: _get_current_item_id() is called
+        result = self.form._get_current_item_id(mocked_list_widget)
+
+        # THEN: The result should be -1
+        mocked_list_widget.currentItem.assert_called_once_with()
+        mocked_item.data.assert_called_once_with(QtCore.Qt.UserRole)
+        assert result == 7
+
+    @patch('openlp.plugins.songs.forms.songmaintenanceform.critical_error_message_box')
+    def test_delete_item_no_item_id(self, mocked_critical_error_message_box):
+        """
+        Test the _delete_item() method when there is no item selected
+        """
+        # GIVEN: Some mocked items
+        mocked_item_class = MagicMock()
+        mocked_list_widget = MagicMock()
+        mocked_reset_func = MagicMock()
+        dialog_title = 'Delete Item'
+        delete_text = 'Are you sure you want to delete this item?'
+        error_text = 'There was a problem deleting this item'
+
+        # WHEN: _delete_item() is called
+        with patch.object(self.form, '_get_current_item_id') as mocked_get_current_item_id:
+            mocked_get_current_item_id.return_value = -1
+            self.form._delete_item(mocked_item_class, mocked_list_widget, mocked_reset_func, dialog_title, delete_text,
+                                   error_text)
+
+        # THEN: The right things should have been called
+        mocked_get_current_item_id.assert_called_once_with(mocked_list_widget)
+        mocked_critical_error_message_box.assert_called_once_with(dialog_title, UiStrings().NISs)
+
+    @patch('openlp.plugins.songs.forms.songmaintenanceform.critical_error_message_box')
+    def test_delete_item_invalid_item(self, mocked_critical_error_message_box):
+        """
+        Test the _delete_item() method when the item doesn't exist in the database
+        """
+        # GIVEN: Some mocked items
+        self.mocked_manager.get_object.return_value = None
+        mocked_item_class = MagicMock()
+        mocked_list_widget = MagicMock()
+        mocked_reset_func = MagicMock()
+        dialog_title = 'Delete Item'
+        delete_text = 'Are you sure you want to delete this item?'
+        error_text = 'There was a problem deleting this item'
+
+        # WHEN: _delete_item() is called
+        with patch.object(self.form, '_get_current_item_id') as mocked_get_current_item_id:
+            mocked_get_current_item_id.return_value = 1
+            self.form._delete_item(mocked_item_class, mocked_list_widget, mocked_reset_func, dialog_title, delete_text,
+                                   error_text)
+
+        # THEN: The right things should have been called
+        mocked_get_current_item_id.assert_called_once_with(mocked_list_widget)
+        self.mocked_manager.get_object.assert_called_once_with(mocked_item_class, 1)
+        mocked_critical_error_message_box.assert_called_once_with(dialog_title, error_text)
+
+    @patch('openlp.plugins.songs.forms.songmaintenanceform.critical_error_message_box')
+    def test_delete_item(self, mocked_critical_error_message_box):
+        """
+        Test the _delete_item() method
+        """
+        # GIVEN: Some mocked items
+        mocked_item = MagicMock()
+        mocked_item.songs = []
+        mocked_item.id = 1
+        self.mocked_manager.get_object.return_value = mocked_item
+        mocked_critical_error_message_box.return_value = QtWidgets.QMessageBox.Yes
+        mocked_item_class = MagicMock()
+        mocked_list_widget = MagicMock()
+        mocked_reset_func = MagicMock()
+        dialog_title = 'Delete Item'
+        delete_text = 'Are you sure you want to delete this item?'
+        error_text = 'There was a problem deleting this item'
+
+        # WHEN: _delete_item() is called
+        with patch.object(self.form, '_get_current_item_id') as mocked_get_current_item_id:
+            mocked_get_current_item_id.return_value = 1
+            self.form._delete_item(mocked_item_class, mocked_list_widget, mocked_reset_func, dialog_title, delete_text,
+                                   error_text)
+
+        # THEN: The right things should have been called
+        mocked_get_current_item_id.assert_called_once_with(mocked_list_widget)
+        self.mocked_manager.get_object.assert_called_once_with(mocked_item_class, 1)
+        mocked_critical_error_message_box.assert_called_once_with(dialog_title, delete_text, self.form, True)
+        self.mocked_manager.delete_object(mocked_item_class, 1)
+        mocked_reset_func.assert_called_once_with()
+
+    @patch('openlp.plugins.songs.forms.songmaintenanceform.QtWidgets.QListWidgetItem')
+    @patch('openlp.plugins.songs.forms.songmaintenanceform.Author')
+    def test_reset_authors(self, MockedAuthor, MockedQListWidgetItem):
+        """
+        Test the reset_authors() method
+        """
+        # GIVEN: A mocked authors_list_widget and a few other mocks
+        mocked_author1 = MagicMock()
+        mocked_author1.display_name = 'John Newton'
+        mocked_author1.id = 1
+        mocked_author2 = MagicMock()
+        mocked_author2.display_name = ''
+        mocked_author2.first_name = 'John'
+        mocked_author2.last_name = 'Wesley'
+        mocked_author2.id = 2
+        mocked_authors = [mocked_author1, mocked_author2]
+        mocked_author_item1 = MagicMock()
+        mocked_author_item2 = MagicMock()
+        MockedQListWidgetItem.side_effect = [mocked_author_item1, mocked_author_item2]
+        MockedAuthor.display_name = None
+        self.mocked_manager.get_all_objects.return_value = mocked_authors
+
+        # WHEN: reset_authors() is called
+        with patch.object(self.form, 'authors_list_widget') as mocked_authors_list_widget:
+            self.form.reset_authors()
+
+        # THEN: The authors list should be reset
+        expected_widget_item_calls = [call('John Wesley'), call('John Newton')]
+        mocked_authors_list_widget.clear.assert_called_once_with()
+        self.mocked_manager.get_all_objects.assert_called_once_with(MockedAuthor)
+        assert MockedQListWidgetItem.call_args_list == expected_widget_item_calls, MockedQListWidgetItem.call_args_list
+        mocked_author_item1.setData.assert_called_once_with(QtCore.Qt.UserRole, 2)
+        mocked_author_item2.setData.assert_called_once_with(QtCore.Qt.UserRole, 1)
+        mocked_authors_list_widget.addItem.call_args_list == [
+            call(mocked_author_item1), call(mocked_author_item2)]
+
+    @patch('openlp.plugins.songs.forms.songmaintenanceform.QtWidgets.QListWidgetItem')
+    @patch('openlp.plugins.songs.forms.songmaintenanceform.Topic')
+    def test_reset_topics(self, MockedTopic, MockedQListWidgetItem):
+        """
+        Test the reset_topics() method
+        """
+        # GIVEN: Some mocked out objects and methods
+        MockedTopic.name = 'Grace'
+        mocked_topic = MagicMock()
+        mocked_topic.id = 1
+        mocked_topic.name = 'Grace'
+        self.mocked_manager.get_all_objects.return_value = [mocked_topic]
+        mocked_topic_item = MagicMock()
+        MockedQListWidgetItem.return_value = mocked_topic_item
+
+        # WHEN: reset_topics() is called
+        with patch.object(self.form, 'topics_list_widget') as mocked_topic_list_widget:
+            self.form.reset_topics()
+
+        # THEN: The topics list should be reset correctly
+        mocked_topic_list_widget.clear.assert_called_once_with()
+        self.mocked_manager.get_all_objects.assert_called_once_with(MockedTopic)
+        MockedQListWidgetItem.assert_called_once_with('Grace')
+        mocked_topic_item.setData.assert_called_once_with(QtCore.Qt.UserRole, 1)
+        mocked_topic_list_widget.addItem.assert_called_once_with(mocked_topic_item)
+
+    @patch('openlp.plugins.songs.forms.songmaintenanceform.QtWidgets.QListWidgetItem')
+    @patch('openlp.plugins.songs.forms.songmaintenanceform.Book')
+    def test_reset_song_books(self, MockedBook, MockedQListWidgetItem):
+        """
+        Test the reset_song_books() method
+        """
+        # GIVEN: Some mocked out objects and methods
+        MockedBook.name = 'Hymnal'
+        mocked_song_book = MagicMock()
+        mocked_song_book.id = 1
+        mocked_song_book.name = 'Hymnal'
+        mocked_song_book.publisher = 'Hymns and Psalms, Inc.'
+        self.mocked_manager.get_all_objects.return_value = [mocked_song_book]
+        mocked_song_book_item = MagicMock()
+        MockedQListWidgetItem.return_value = mocked_song_book_item
+
+        # WHEN: reset_song_books() is called
+        with patch.object(self.form, 'song_books_list_widget') as mocked_song_book_list_widget:
+            self.form.reset_song_books()
+
+        # THEN: The song_books list should be reset correctly
+        mocked_song_book_list_widget.clear.assert_called_once_with()
+        self.mocked_manager.get_all_objects.assert_called_once_with(MockedBook)
+        MockedQListWidgetItem.assert_called_once_with('Hymnal (Hymns and Psalms, Inc.)')
+        mocked_song_book_item.setData.assert_called_once_with(QtCore.Qt.UserRole, 1)
+        mocked_song_book_list_widget.addItem.assert_called_once_with(mocked_song_book_item)