Flask-SQLAlchemy models and unit-testing

Question

This is my first time building anything remotely like a robust database. I'm midway through designing it and I would be grateful if you have suggestions or have found any logical errors.

Goal: allow authenticated users to enter data for multiple hospitals.

My database has three tables: User, Hospital, and Data.

A User might have many Hospitals (and vice-versa). Because of this, I have created a many-to-many scheme. A Hospital might have many Datas so I created a one to many relationship.

from petalapp import db

from datetime import datetime
ROLE_USER = 0
ROLE_ADMIN = 1

#TODO:rename?
hospitals = db.Table('hospitals',
    db.Column('hospital_id', db.Integer, db.ForeignKey('hospital.id')),
    db.Column('user_id', db.Integer, db.ForeignKey('user.id'))
)
# tags bmarks time
class User(db.Model):
    """User has a many-to-many relationship with Hospital"""

    id = db.Column(db.Integer, primary_key=True)
    nickname = db.Column(db.String(64), unique = True)
    email = db.Column(db.String(150), unique=True)
    role = db.Column(db.SmallInteger, default=ROLE_USER)

    hospitals = db.relationship('Hospital', secondary=hospitals,
        backref=db.backref('users', lazy='dynamic'))

    def __init__(self, nickname, email, role=ROLE_USER):
        self.nickname= nickname
        self.role = role
        self.email = email

    #TODO what information to show?
    def __repr__(self):
        return '<Name : %r>' % (self.nickname)

    def is_authenticated(self):
        return True

    def is_active(self):
        return True

    def is_anonymous(self):
        return False

    def get_id(self):
        return unicode(self.id)

    def add_hospital(self, hospital):
        if not (hospital in self.hospitals):
            self.hospitals.append(hospital)

    def remove_hospital(self, hospital):
        if not (hospital in self.hospital):
            self.hospitals.remove(hospital)



    @staticmethod
    def make_unique_nickname(nickname):
        if User.query.filter_by(nickname = nickname).first() == None:
            return nickname
        version = 2
        while True:
            new_nickname = nickname + str(version)
            if User.query.filter_by(nickname = new_nickname).first() == None:
                break
            version += 1
        return new_nickname


class Hospital(db.Model):
    """Hospital's has a one-to-many relationship with DATA and a
    many-to-many relationship with User"""

    id = db.Column(db.Integer, primary_key=True)
    name = db.Column(db.String(80))
    data = db.relationship('Data', backref='hospital', lazy = 'dynamic')

    def __init__(self, name):
        self.name = name

    def __repr__(self):
        return '<Name %r>' % self.name


class Data(db.Model):
    """Data has a many-to-one relationship with Hospital"""

    id = db.Column(db.Integer, primary_key=True)

    standard_form = db.Column(db.Integer)
    marketing_education = db.Column(db.Integer)
    record_availability = db.Column(db.Integer)
    family_centerdness = db.Column(db.Integer)
    pc_networking = db.Column(db.Integer)
    education_and_training = db.Column(db.Integer)
    team_funding = db.Column(db.Integer)
    coverage = db.Column(db.Integer)
    pc_for_expired_pts = db.Column(db.Integer)
    hospital_pc_screening  = db.Column(db.Integer)
    pc_follow_up = db.Column(db.Integer)
    post_discharge_services = db.Column(db.Integer)
    bereavement_contacts = db.Column(db.Integer)
    certification = db.Column(db.Integer)
    team_wellness = db.Column(db.Integer)
    care_coordination = db.Column(db.Integer)

    timestamp = db.Column(db.DateTime)

    hospital_id = db.Column(db.Integer, db.ForeignKey('hospital.id'))

    def __init__(self, standard_form=0,
            marketing_education=0, record_availability=0, family_centerdness=0,
        pc_networking=0, education_and_training=0, team_funding=0,
        coverage=0, pc_for_expired_pts=0, hospital_pc_screening=0,
        pc_follow_up=0, post_discharge_services=0, bereavement_contacts=0,
        certification=0, team_wellness=0, care_coordination=0, timestamp=datetime.utcnow()):

            self.standard_form = standard_form
            self.marketing_education =  marketing_education
            self.record_availability = record_availability
            self.family_centerdness =  family_centerdness
            self.pc_networking =  pc_networking
            self.education_and_training =  education_and_training
            self.team_funding =  team_funding
            self.coverage =  coverage
            self.pc_for_expired_pts =  pc_for_expired_pts
            self.hospital_pc_screening  =  hospital_pc_screening
            self.pc_follow_up =  pc_follow_up
            self.post_discharge_services =  post_discharge_services
            self.bereavement_contacts =  bereavement_contacts
            self.certification =  certification
            self.team_wellness =  team_wellness
            self.care_coordination = care_coordination
            self.timestamp = timestamp

    def __repr__(self):
        return """
    <standard_form : %r>\n
    <marketing_education : %r>\n
    <record_availability : %r>\n
    <family_centerdness : %r>\n
    <pc_networking : %r>\n
    <education_and_training : %r>\n
    <team_funding : %r>\n
    <coverage : %r>\n
    <pc_for_expired_pts : %r>\n
    <hospital_pc_screening  : %r>\n
    <pc_follow_up : %r>\n
    <post_discharge_services : %r>\n
    <bereavement_contacts : %r>\n
    <certification : %r>\n
    <team_wellness : %r>\n
    <care_coordination : %r>\n
    <datetime_utc  : %r>""" % (
    self.standard_form,
    self.marketing_education,
    self.record_availability,
    self.family_centerdness,
    self.pc_networking,
    self.education_and_training,
    self.team_funding,
    self.coverage,
    self.pc_for_expired_pts,
    self.hospital_pc_screening ,
    self.pc_follow_up,
    self.post_discharge_services,
    self.bereavement_contacts,
    self.certification,
    self.team_wellness,
    self.care_coordination,
    self.timestamp)

---

Here are my tests so far:

'''
File: db_test_many_create.py
Date: 2012-12-06
Author: Drew Verlee
Description: functions to build a many-to-many relationship
'''
import unittest
from petalapp.database.models import User, Hospital, Data, ROLE_USER
from petalapp import db

class BuildDestroyTables(unittest.TestCase):

    def setUp(self):
        db.drop_all()
        db.create_all()


    def tearDown(self):
        db.session.remove()
        db.drop_all()


    def test_user_setup(self):

        user_test_1 = User("test_user_nickname","user_email",ROLE_USER)
        db.session.add(user_test_1)
        db.session.commit()


    def test_data_setup(self):

        data_test_1 = Data(1)
        db.session.add(data_test_1)
        db.session.commit()

    def test_hospital_setup(self):

        hospital_test_1 = Hospital("test_hospital_1")
        db.session.add(hospital_test_1)
        db.session.commit()


    def test_make_unique_nickname(self):
        u = User(nickname = 'john', email = 'john@example.com')
        db.session.add(u)
        db.session.commit()
        nickname = User.make_unique_nickname('john')
        assert nickname != 'john'
        u = User(nickname = nickname, email = 'susan@example.com')
        db.session.add(u)
        db.session.commit()
        nickname2 = User.make_unique_nickname('john')
        assert nickname2 != 'john'
        assert nickname2 != nickname


    def test_multiple_link_table(self):


        #create
        drew = User(nickname= "Drew", email="Drew@gmail.com",role=ROLE_USER)
        mac_hospital = Hospital("Mac_hospital")
        pro_hospital = Hospital("pro_hospital")
        mac_data = Data(1,2,3)
        pro_data = Data(10,9,8)

        #add
        db.session.add(drew)
        db.session.add(mac_hospital)
        db.session.add(pro_hospital)
        db.session.add(mac_data)
        db.session.add(pro_data)

        #commit
        db.session.commit()

        #create links
        mac_hospital.data.append(mac_data)
        pro_hospital.data.append(pro_data)
        drew.hospitals.append(mac_hospital)
        drew.hospitals.append(pro_hospital)
        db.session.commit()

    def functions_of_add_remove(self):
        johns_hospital_data = Data('johns_hospital_data')
        johns_hospital = Hospital('johns_hospital')
        john = User('john', 'john@gmail.com')

        db.session.add(johns_hospital_data)
        db.session.add(johns_hospital)
        db.session.add(john)

        #TODO make a function for this?
        johns_hospital.append(johns_hospital_data)
        #do  i need a commit?
        db.session.commit()

        self.assertEqual(john.remove_hospital(johns_hospital), None)

        john_has_hospital = john.add_hospital(johns_hospital)
        db.session.add(john_has_hospital)
        db.session.commit()

        self.assertEqual(john.add_hospital(johns_hospital), None)
        self.assertEqual(len(john.hospitals), 1)


if __name__ == "__main__":
    unittest.main()

Answer 1

Models - General

The comments on the model definitions don't add much value. I could figure out the relationships from the db schema; tell me instead something about what the classes are meant to represent, any invariants they have, anything not made explicit in the code.

Models - User

When defining a backreference, as you do in User, I like to go to the other class and add a comment saying something along the lines of "A backreference 'users' is created to the Users class". With that said, I would avoid naming the variable users; what does the variable name 'users' tell me when reading the class? It tells me the type of the object I'm getting, but nothing about what significance those objects have. Something like registered_users, patients (or whatever makes sense) might be a better option.

You could replace your is_active, is_anonymous methods with read only properties, but that's personal preference. The get_id method seems very odd, not sure I see the value.

I question also the value of the add_hospital and remove_hospital methods, though that may be due to lack of knowledge of your use case. Regardless, the remove_hospital is incorrect; it should be:

def remove_hospital(self, hospital):
    if hospital in self.hospitals: # You have if not (hospital in self.hospital)
        self.hospitals.remove(hospital)

Models - Data

Personally, any variable named data sets off alarm bells. As it is, the Data class seems to store a lot of "stuff", and no where do I get told why this stuff should be grouped together, what its conceptual value is, etc etc. Would something like InspectionResult (or whatever the data comes from) be more useful? Regardless, I can't help but feel this class does too much.

Your __init__ for the Data class is horrendous. Use the default parameter of the Column constructor to set defaults, and make sure to call Super(Data, self).__init__() if you are overriding the __init__ of a subclass of anything you don't control.

Tests

I can't say I like the name of the test class; it calls itself BuildDestroyTables and then goes on to do a whole lot more than that. I've personally moved away from class based testing, but back when I used it, I would define a BaseDatabaseTest class that other tests could inherit from to absolve them of having to worry about database setup/cleanup while not violating the single responsibility principle.

You use a raw sessions; I would encourage you to use a contextmanager or something to manage your session. Here's one from a project I'm currently working on:

import contextlib

@contextlib.contextmanager
def managed_session(**kwargs):
    """
    Provides "proper" session management to a block. Features:

     - Rollback if an exception is raised
     - Commit if no exception is raised
     - Session cleanup on exit
    """
    session = db_session(**kwargs) # Or however you create a new session
    try:
        yield session
    except:
        session.rollback()
        raise
    else:
        session.commit()
    finally:
        session.close()

You can then use it like this:

with managed_session() as session:
    session.add(foo)
    raise RuntimeException("foobar")
    # But it's ok, everything will get rolled back nicely and we'll have a viable session still

Stuff like the automatic commit is not to everyone's taste, but I think in general, the exception safety is nice. With that said, this is probably the most controversial thing I'll say in this answer; there are many approaches to session management, and this is only one.