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 = '[email protected]')
 db.session.add(u)
 db.session.commit()
 nickname = User.make_unique_nickname('john')
 assert nickname != 'john'
 u = User(nickname = nickname, email = '[email protected]')
 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="[email protected]",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', '[email protected]')
 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()

• kobejohn: I'm looking for exactly the same kind of best practice. If you have become confident in how to do it, could you share in a self-answer?

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.