openlp-core team mailing list archive

[Jon Tibble <meths@xxxxxxxxxxxxxx>]

Jon Tibble has proposed merging lp:~meths/openlp/trivialfixes into lp:openlp.

Requested reviews:
  OpenLP Core (openlp-core)

For more details, see:
https://code.launchpad.net/~meths/openlp/trivialfixes/+merge/46434

Fix up song validating and saving.

The previous fixes were avoiding the real issue in how the validating and saving methods were interacting with the database.  We need to keep the different items separate.  SQLAlchemy can keep track of relationships but doesn't know an item (in this case a song) is incomplete when it flushes/commits the database because we save another item (an author, book or topic).
-- 
https://code.launchpad.net/~meths/openlp/trivialfixes/+merge/46434
Your team OpenLP Core is requested to review the proposed merge of lp:~meths/openlp/trivialfixes into lp:openlp.

=== modified file 'openlp/plugins/songs/forms/editsongform.py'
--- openlp/plugins/songs/forms/editsongform.py	2011-01-15 20:06:25 +0000
+++ openlp/plugins/songs/forms/editsongform.py	2011-01-17 01:34:11 +0000
@@ -332,7 +332,7 @@
                 else:
                     author = Author.populate(first_name=text.rsplit(u' ', 1)[0],
                         last_name=text.rsplit(u' ', 1)[1], display_name=text)
-                self.manager.save_object(author, False)
+                self.manager.save_object(author)
                 author_item = QtGui.QListWidgetItem(
                     unicode(author.display_name))
                 author_item.setData(QtCore.Qt.UserRole,
@@ -386,7 +386,7 @@
                 QtGui.QMessageBox.Yes | QtGui.QMessageBox.No,
                 QtGui.QMessageBox.Yes) == QtGui.QMessageBox.Yes:
                 topic = Topic.populate(name=text)
-                self.manager.save_object(topic, False)
+                self.manager.save_object(topic)
                 topic_item = QtGui.QListWidgetItem(unicode(topic.name))
                 topic_item.setData(QtCore.Qt.UserRole,
                     QtCore.QVariant(topic.id))
@@ -524,12 +524,13 @@
 
     def _validate_song(self):
         """
-        Check the validity of the form. Only display the 'save' if the data
-        can be saved.
+        Check the validity of the song.
         """
+        # This checks data in the form *not* self.song.  self.song is still
+        # None at this point.
         log.debug(u'Validate Song')
         # Lets be nice and assume the data is correct.
-        if len(self.titleEdit.displayText()) == 0:
+        if not self.titleEdit.text():
             self.songTabWidget.setCurrentIndex(0)
             self.titleEdit.setFocus()
             criticalErrorMessageBox(
@@ -550,9 +551,9 @@
                 message=translate('SongsPlugin.EditSongForm',
                 'You need to have an author for this song.'))
             return False
-        if self.song.verse_order:
+        if self.verseOrderEdit.text():
             order = []
-            order_names = self.song.verse_order.split()
+            order_names = self.verseOrderEdit.text().split()
             for item in order_names:
                 if len(item) == 1:
                     order.append(item.lower() + u'1')
@@ -593,6 +594,19 @@
                         QtGui.QMessageBox.Yes | QtGui.QMessageBox.No)
                     if answer == QtGui.QMessageBox.No:
                         return False
+        item = int(self.songBookComboBox.currentIndex())
+        text = unicode(self.songBookComboBox.currentText())
+        if self.songBookComboBox.findText(text, QtCore.Qt.MatchExactly) < 0:
+            if QtGui.QMessageBox.question(self,
+                translate('SongsPlugin.EditSongForm', 'Add Book'),
+                translate('SongsPlugin.EditSongForm', 'This song book does '
+                'not exist, do you want to add it?'),
+                QtGui.QMessageBox.Yes | QtGui.QMessageBox.No,
+                QtGui.QMessageBox.Yes) == QtGui.QMessageBox.Yes:
+                book = Book.populate(name=text, publisher=u'')
+                self.manager.save_object(book)
+            else:
+                return False
         return True
 
     def onCopyrightInsertButtonTriggered(self):
@@ -653,33 +667,25 @@
         """
         log.debug(u'accept')
         self.clearCaches()
-        if not self.song:
-            self.song = Song()
-        item = int(self.songBookComboBox.currentIndex())
-        text = unicode(self.songBookComboBox.currentText())
-        if self.songBookComboBox.findText(text, QtCore.Qt.MatchExactly) < 0:
-            if QtGui.QMessageBox.question(self,
-                translate('SongsPlugin.EditSongForm', 'Add Book'),
-                translate('SongsPlugin.EditSongForm', 'This song book does '
-                'not exist, do you want to add it?'),
-                QtGui.QMessageBox.Yes | QtGui.QMessageBox.No,
-                QtGui.QMessageBox.Yes) == QtGui.QMessageBox.Yes:
-                book = Book.populate(name=text, publisher=u'')
-                self.manager.save_object(book, False)
-            else:
-                return
-        if self.saveSong():
+        if self._validate_song():
+            self.saveSong()
             Receiver.send_message(u'songs_load_list')
             self.close()
 
     def saveSong(self, preview=False):
         """
         Get all the data from the widgets on the form, and then save it to the
-        database.
+        database.  The form has been validated and all reference items
+        (Authors, Books and Topics) have been saved before this function is
+        called.
 
         ``preview``
             Should be ``True`` if the song is also previewed (boolean).
         """
+        # The Song() assignment.  No database calls should be made while a
+        # Song() is in a partially complete state.
+        if not self.song:
+            self.song = Song()
         self.song.title = unicode(self.titleEdit.text())
         self.song.alternate_title = unicode(self.alternativeEdit.text())
         self.song.copyright = unicode(self.copyrightEdit.text())
@@ -703,27 +709,27 @@
             self.song.theme_name = theme_name
         else:
             self.song.theme_name = None
-        if self._validate_song():
-            self.processLyrics()
-            self.processTitle()
-            self.song.authors = []
-            for row in range(self.authorsListView.count()):
-                item = self.authorsListView.item(row)
-                authorId = (item.data(QtCore.Qt.UserRole)).toInt()[0]
-                self.song.authors.append(self.manager.get_object(Author,
-                    authorId))
-            self.song.topics = []
-            for row in range(self.topicsListView.count()):
-                item = self.topicsListView.item(row)
-                topicId = (item.data(QtCore.Qt.UserRole)).toInt()[0]
-                self.song.topics.append(self.manager.get_object(Topic, topicId))
-            self.manager.save_object(self.song)
-            if not preview:
-                self.song = None
-            return True
-        return False
+        self.processLyrics()
+        self.processTitle()
+        self.song.authors = []
+        for row in range(self.authorsListView.count()):
+            item = self.authorsListView.item(row)
+            authorId = (item.data(QtCore.Qt.UserRole)).toInt()[0]
+            self.song.authors.append(self.manager.get_object(Author, authorId))
+        self.song.topics = []
+        for row in range(self.topicsListView.count()):
+            item = self.topicsListView.item(row)
+            topicId = (item.data(QtCore.Qt.UserRole)).toInt()[0]
+            self.song.topics.append(self.manager.get_object(Topic, topicId))
+        self.manager.save_object(self.song)
+        if not preview:
+            self.song = None
 
     def processLyrics(self):
+        """
+        Process the lyric data entered by the user into the OpenLP XML format.
+        """
+        # This method must only be run after the self.song = Song() assignment.
         log.debug(u'processLyrics')
         try:
             sxml = SongXML()
@@ -749,6 +755,11 @@
                 sxml.dump_xml())
 
     def processTitle(self):
+        """
+        Process the song title entered by the user to remove stray punctuation
+        characters.
+        """
+        # This method must only be run after the self.song = Song() assignment.
         log.debug(u'processTitle')
         self.song.search_title = re.sub(r'[\'"`,;:(){}?]+', u'',
             unicode(self.song.search_title)).lower()

=== modified file 'openlp/plugins/songs/lib/db.py'
--- openlp/plugins/songs/lib/db.py	2011-01-14 22:17:46 +0000
+++ openlp/plugins/songs/lib/db.py	2011-01-17 01:34:11 +0000
@@ -72,7 +72,7 @@
     ``url``
         The database to setup
     """
-    session, metadata = init_db(url, False)
+    session, metadata = init_db(url)
 
     # Definition of the "authors" table
     authors_table = Table(u'authors', metadata,