Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add sqlite database #5

Merged
merged 11 commits into from
Oct 11, 2014
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ Flask==0.9
twilio==3.3.10
mock==0.8.0
nose==1.1.2
sqlalchemy==0.9.1
122 changes: 122 additions & 0 deletions src/sqlite_phone_directory.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍 Some minor comments.

import datetime
import re
import sqlalchemy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import nitpicks:

  1. Inconsistent imports here. Maybe add create_engine to line 6 and delete line 5.
  2. Maybe alphabetize imports for better readability

from sqlalchemy import Boolean, Column, Integer, String, Text, DateTime, ForeignKey
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.orm import sessionmaker, Session

from phone_directory_actions import PhoneDirectoryActions
from util import validate_phone_number


# Creating the base class for database mapped objects,
# see: http://docs.sqlalchemy.org/en/rel_0_9/orm/extensions/declarative.html
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good comment!

Base = declarative_base()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's happening here?


# A helper for binding a database engine for use with declaritive base
def bind_engine(engine):
Base.metadata.create_all(engine)
return sessionmaker(bind=engine, autoflush=True)

class User(Base):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring, please

""" User model representing a user with a user_id, name, and phone number """
__tablename__ = 'users'

user_id = Column('user_id', Integer, primary_key=True)
phone = Column('crm_type', String(100))
name = Column('name', String(100))
updated_at = Column('updated_at', DateTime, onupdate=datetime.datetime.now, default=datetime.datetime.now, index=True)
created_at = Column('created_at', DateTime, default=datetime.datetime.now, index=True)


class PhoneHistory(Base):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring please!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

""" PhoneHistory, we store a PhoneHistory entry for each change """
__tablename__ = 'phone_history'
id = Column('id', Integer, index=True, primary_key=True)
user_id = Column('user_id', Integer, index=True)
phone = Column('phone', String(100))
updated_at = Column('updated_at', DateTime, onupdate=datetime.datetime.now, default=datetime.datetime.now, index=True)
created_at = Column('created_at', DateTime, default=datetime.datetime.now, index=True)


class SqlitePhoneDirectoryActions(PhoneDirectoryActions):
'''
Data stored in a sqliteDB.
'''

def __init__(self, database="sqlite:///:memory:"):
engine = sqlalchemy.create_engine(
database # TODO: better path to database
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For each TODO checked in, make a corresponding issue describing exactly what you mean is to be done. Refer to the issue number in the TODO comment.

)

self._SessionMaker = bind_engine(engine)

def _get_user_by_id(self, user_id):
session = self._SessionMaker()
users_returned = session.query(User).filter(User.user_id == user_id).all()
session.close()

if len(users_returned) != 1:
raise KeyError("User {} not found".format(user_id))
else:
return users_returned[0]

def user_exists(self, user_id):
'''
Return true if user_id exists, false if not
'''
user = self._get_user_by_id(user_id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure that _get_user_by_id returns a value if user_id doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it raises an exception. KeyError

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a bad user experience for someone using the twilio app. Better to catch the exception.

It's a
image as soon as this is fixed.

return (user != None) # truthy

def get_phone_number(self, user_id):
'''
Return phone number for given user_id. Throw KeyError if user doesn't exist.
'''
user = self._get_user_by_id(user_id)
return user.phone

def get_name(self, user_id):
'''
Return username for given user_id. Throw KeyError if user doesn't exist.
'''
user = self._get_user_by_id(user_id)
return user.name

def update_phone_number(self, user_id, new_number):
'''
If user_id exists, update its phone number and archive the old number.
If user_id does not exist, throw KeyError
If new phone number is not valid, throw ValueError
'''
user = self._get_user_by_id(user_id)
validate_phone_number(new_number)
old_number = user.phone
user.phone = new_number

session = self._SessionMaker()

history = PhoneHistory()
history.user_id = user.user_id
history.phone = old_number

session.add(user)
session.add(history)
session.commit()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this session need to be closed? Or does commit close it too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

session.close()

def _add_a_user(self, user_id, phone, name):
validate_phone_number(phone)

session = self._SessionMaker()

user = User()
user.name = name
user.user_id = user_id
user.phone = phone

session.add(user)
session.commit()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this session need to be closed? Or does commit close it too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, I missed that.

session.close()


Empty file added test/.gitigore
Empty file.
28 changes: 28 additions & 0 deletions test/test_sqlite_phone_directory.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import unittest
import sys

sys.path.append("src")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the ideal method, but will work fine if people are using nosetests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay testing! Could you update the readme saying how to run these tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.


from sqlite_phone_directory import SqlitePhoneDirectoryActions

class TestSqlitePhoneDirectory(unittest.TestCase):

def setUp(self):
self._db = SqlitePhoneDirectoryActions(database="sqlite:///:memory:")
self._db._add_a_user(user_id=1122, phone='1'*10, name='ben')

def test_user_exists(self):
assert self._db.user_exists(1122)

def test_get_name(self):
assert self._db.get_name(1122) == 'ben'

def test_get_phone_number(self):
assert self._db.get_phone_number(1122) == '1'*10

def test_update_phone_number(self):
self._db.update_phone_number(user_id=1122, new_number='2'*10)
assert self._db.get_phone_number(1122) == '2'*10