Freeseer / freeseer

Designed for capturing presentations at conferences. Pre-fill a list of talks to record, record them, and upload them to YouTube with our YouTube Uploader.
http://freeseer.readthedocs.org
GNU General Public License v3.0
215 stars 110 forks source link

unit test and refactor database.py part 2 #671

Open SRomansky opened 9 years ago

SRomansky commented 9 years ago

Add unit tests and refactor/fix/document issues related to the database. (Part 2, because #619 grew too large.)

Related #484 Related #619

SRomansky commented 9 years ago

@dideler @zxiiro https://gist.github.com/wigglier/9591f5d05d4752f65d41 this might be the culprit for why the database appears to have NULLs instead of empty-string characters.

edit: This is from adding a print statement to insert_presentation()

SRomansky commented 9 years ago
    'Title,Speaker,Abstract,Category,Event,Room,Date,StartTime,EndTime\r\n',
        'Managing map data in a database,Andrew Ross,,,Default,Default,,,\r\n',
        'Building NetBSD,David Maxwell,,,Default,Default,,,\r\n',
        'Faking it till you make it,John Doe,,,Default,Default,,,\r\n'

From the expected csv lines. Note that the ,, is like None pretty much. So SQL isn't inserting '' at all. instead it is taking the content from the string objects (which is nothing) and inserting it into the prepared presentation fields in database.py. Hence, we get None in the db.

SRomansky commented 9 years ago
    def _helper_presentation_exists(self, presentation):
        """This is used in testing. Checks if there's a presentation with the same Speaker and Title already stored."""
        query = QtSql.QSqlQuery()

        query.prepare('''                                                                                             
            SELECT *                                                                                                  
            FROM presentations                                                                                        
            WHERE Title=:title AND Speaker=:speaker AND Description=:description AND Category=:category AND           
                  Event=:event AND (Room is NULL OR Room=:room) AND (Date=:date OR Date is NULL) AND                  
                  (StartTime=:startTime or StartTime is Null) AND EndTime=:endTime                                    
        ''')
        query.bindValue(':title', presentation.title)
        query.bindValue(':speaker', presentation.speaker)
        query.bindValue(':description', presentation.description)
        query.bindValue(':category', presentation.category)
        query.bindValue(':event', presentation.event)
        query.bindValue(':room', presentation.room)
        query.bindValue(':date', presentation.date)
        query.bindValue(':startTime', presentation.startTime)
        query.bindValue(':endTime', presentation.endTime)
        query.exec_()
        print query.boundValue(':room').toString()

        if query.next():
            return True
        else:
            return False

This should be equivalent to what is already in database.py but it is not for some reason

Specifically when presentation.room='' this fails.

SRomansky commented 9 years ago

@dideler https://www.sqlite.org/datatype3.html in section 1.2 it mentions that SQLITE does not have a DATE or a TIME type and instead just has that timestamp one. :(.

dideler commented 9 years ago

image

The first record uses the latest changes on this branch. The second record is before the changes introduced in 43d892f. The last record uses the code below.

def create_presentation(self, talkDetailsWidget):
    """Creates and returns an instance of Presentation using data from the input fields"""
    title = unicode(talkDetailsWidget.titleLineEdit.text()).strip()
    if title:
        return Presentation(
            title,
            unicode(talkDetailsWidget.presenterLineEdit.text()).strip(),
            unicode(talkDetailsWidget.descriptionTextEdit.toPlainText()).strip(),
            unicode(talkDetailsWidget.categoryLineEdit.text()).strip(),
            unicode(talkDetailsWidget.eventLineEdit.text()).strip(),
            unicode(talkDetailsWidget.roomLineEdit.text()).strip(),
            talkDetailsWidget.dateEdit.date(),
            talkDetailsWidget.startTimeEdit.time().toString("hh:mm ap"),
            talkDetailsWidget.endTimeEdit.time().toString("hh:mm ap"))
dideler commented 9 years ago

Exporting talks to CSV and then importing them gets rid of the date and start/end times. Haven't checked an earlier version to see if it worked, but if it did, this is a regression and will need to be fixed before merging.

SRomansky commented 9 years ago

@dideler the missing dates on import seems to occur in master too.

SRomansky commented 9 years ago

@dideler I think the csv cannot be imported because the exporter uses StartTime,EndTime in its output and the import utility looks for only a Time field.

dideler commented 9 years ago

@wigglier you asked on IRC if this should be split up into smaller commits.

I think it would help and shouldn't be too difficult. You can group files into new PRs, for example:

# Create new branch for new PR
git checkout -b refactor-rss-stuff-for-tests 484-unit-test-and-refactor-p2

# Uncommit the single squashed commit
git reset --soft HEAD^

# Unstage the files you want in the new PR
git reset src/freeseer/tests/plugins/importer/rss_feedparser/test_rss_feedparser.py \
    src/freeseer/tests/resources/sample_rss_data/sc2011.rss \
    src/freeseer/tests/resources/sample_rss_data/summercamp2010.json  \
    src/freeseer/tests/resources/sample_rss_data/summercamp2010.rss \
    src/freeseer/tests/plugins/importer/rss_feedparser/presentation_feed.json \
    src/freeseer/tests/plugins/importer/rss_feedparser/presentation_feed.rss

# Temporarily commit all the other changes
git commit -m 'WIP'

# Stage and commit all the RSS stuff 
git add .
git commit

# Remove the WIP commit so this branch only has the RSS changes commit
git rebase -i HEAD^^  # Just delete the line with the WIP commit
SRomansky commented 9 years ago

679 created, @dideler

SRomansky commented 9 years ago

@dideler when #679 gets merged, I can remove the rss related files from this PR. I can split the 1 giant commit into smaller pieces too, e.g. one commit for each file or two added to tests/framework/database

SRomansky commented 9 years ago

@dideler I removed the code that stored the time format as 'hh:mm ap' and just kept it as 'hh:mm:ss'. The first problem from using 'hh:mm ap' is that the database stores a different value than what is in csv files and what the QTime object uses as its default time format. So there are extra function calls required to convert from hh:mm:ss -> hh:mm ap and back again.

The second problem is that I am not sure how the database is going to handle sorting timestamp values with entries like '3:52 pm' instead of '15:52:00'. In Python if the string '3:52 pm' is cast into the QTime object with out a format specified pyqt will just cast it to the null QTime object. Therefore, I am not sure how SQLITE will handle '3:52 pm' instead of the default time format.

dideler commented 9 years ago

What about displaying the time in a human friendly format but saving it however is easiest in the DB?

SRomansky commented 9 years ago

@dideler I think that can be done. I will investigate how.