Flask-SQLAlchemy models and unit-testing

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 '' % (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

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.

StackExchange Code Review Q#20008, answer score: 12