An external review of tables.py

[spt]

I am a friend of Kevin Landers. He has been talking about this
project since it was an amorphous idea. We were talking about it
again a few days ago. I spent a few minutes today taking a look at
the website and then took a look at the schema as defined in
tables.py.

I understand that this project is still in the design phase. I don't
know how you feel about external feedback especially at this
juncture. I don't mean to step on any toes or to cause problems or to
criticize.

Anyway, here is my comments...


General Schema Comments:

* Could you comment each table with its purpose?

* If I understand the project entailment, this project could have a
very large schema. It might not be a bad idea to graph the schema (and
relationships)

* Could you include a readme that gives the dependencies and urls,
doc urls, etc in the trunk (python, pylon, sqlalchemy, etc)

* I didn't see how Foreign Keys are handled (ie, on delete, on
update, etc)?

* I would add more default values to the schema, ie on non-null
varchars add a default ''. I am uncertain about how this is handled
in this stack but with many app stacks this could be troublesome. This
would be a good cross-database practice as the behavior sometimes
changes.

* Explicitly what database or databases are going to be supported?
Are you going to support all that sqlalchemy supports? A subset or
just one?

* I didn't see any application level data, ie configs, preferences,
etc

* I might recommend that every table have a created timestamp and
if the values can change then a modified timestamp as well. (ie, no
modified on pure relationship tables).

* I would recommend increasing the size of your varchars

* I would recommend the use of check constraints to validate &
sanitize data



Table Specific Comments:

MEMBERS

* Comment doesn't match the table name

* I generally recommend a disabled field in order to turn off auth
but still keep history and relationships

* What should be the checks in place on emails? (ie, number of
chars, has a '@', etc)?

* If you use email address as username, then email needs to be a
unique.

* Is the password encrypted?

* What should be the checks in place on passwords? Is nullable
enough? Is '' acceptable? Shouldn't this have a certain number of
chars (6 min)?

* How are you going to handle forgot password style requests? Ie,
email them with random one, password hit or reset question & answer?

* last_login is set to default to now; this is incorrect behavior
and greatly reduces usefulness of field.

* Are you going to display full email or first & last name in
application? Or should you have a short name?

* Add Description?

* Add Title?

* Add Modified?


ROLES

* I might add a short description and then make description much
longer (maybe just a text field)

* Name should be unique?

* Add Created?

* Add Modified?


MEMBERS_ROLES

* I would recommend a unique contraint over member_id & role_id, or
a composite primary key?

* Minimal number of chars?

* Add Created?


PERMISSIONS

* Name should be unique?

* Minimal number of chars?

* Add Created?


PERMISSIONS_ROLES

* I would recommend a unique contraint over permission_id &
role_id, or a composite primary key?

* Add Created?

CHURCHES

* Are multiple churches going to be handled?

* Multiple Locations?

* Are you going to support additional phones?

* I might recommend a long and short version of name..

* Denoniation?

* Should Addresses be split into associated parts?

* Add Pastor/Minister?

* Add Description?

* Add Welcome or About us?

* Add Created?

* Add Modified?

* Constraints on Name? Unique, length, etc?


SONGS

* Add Created?

* Add Modified?

* Shouldn't hymnal be split into another table? I know churches
that have multiple hymnals. It seems like adding page number in the
relationship might also be helpful? Should the name be changed to
something more generic to include songbooks, handouts, etc?

* Are all hymn_numbers integers for all hymnals? I seem to
remember a church that would have hyphens in it and another that would
occasional have a alternate version (ie, same hymn but with different
lyrics or music) with a letter at the end.

* If you have lyrics, what about splitting into stanza's? I know
alot of people do first stanza of Amazing grace and maybe the last but
may skip the middle.

* What is presentation_order, this seems like it shouldn't be here
at all. It should go into a relationship of songs.

* Any constraints, not null, defaults, minimal chars, etc?

* Should date written be added to song? ie, could be separate from
copyright?

* Should a description be added?

* Should notes be added?


FILES

* What is type vs filetype? Does type categorize the files? If
so, I would decouple and create a file type/category/classification
table.

* Add mimetype? Sometimes you have to tickle the browser so it
knows how to handle.

* Add Created?

* Add Modified?

* Any path info? Are you going to allow duplicate filenames?

* Type is indexable but filename isn't?

TOPICS

* Add Created?

* Add Description?

* Unique name, minimal chars?

SONGS_TOPICS

* Add Created?


AUTHORS

* Add Created?

* Add Modified?

* Add Born?

* Add Death?

* Add Bio?

* Should name be split into parts?


AUTHORS_SONGS

* I would recommend a unique constraint over song_id & topic_id, or
a composite primary key?

* Add Created?


BIBLES

* Is version = name? Add Name?

* Should you have a unique on version and/or name?

* Minimal characters?

* Description?

* Is Copyright = Publish Date?

TESTAMENTS

* Need to add sort order or ordering field.

* Short & Long name?

BOOKS

* Need to add sort order or ordering field.

* Author?

* Description?

* Original Language?

* Time Period?


BIBLES_BOOKS

* This seems useless?

VERSES

* Add description?

* Should any information about markup be used? Ie, bolding, red
for Jesus' words, etc?

* Unique contraint over bible, book, chapter, verse?

* text is a reserved database word. I would recommend renaming.


BIBLE_REFERENCES

* Add Created?

* Add Modified?

* Reference should be unique?

* Minimal characters?

* text is a reserved database word. I would recommend renaming.


SERVICES

* service_date shouldn't default to now.

* Not all services has a "preacher"...?

* Add Stop Date?

* Hum... see below...

PREACHERS
WORSHIP_GROUPS
CAPACITIES
FUNCTIONS
WORSHIP_GROUP_MEMBERS
SERVICE_MEMBERS


Questions:

* Does ForeignKeys in sqlalchemy implicitly add indexes on the
field?

* Is anything going to be logged ie, who added, modified, etc
what? How is this going to occur?

* What are song's key_line, ccli_number, catalog, keys, capo,
time_sig, etc?

* What is the purpose of bible_references?

* I am uncertain about capacities & functions what do these
actually represent? How are they going to be used? What is the
difference between them?

* It seems like capacities & functions are used for roles?

* What are service groups and worship groups?


Design:

* How are the permissions to be handled? The permissions from the
schema doesn't look like they do anything or imply anything at the DD
level. I would have expected to see some booleans that defined
specific access, ie, read (x), write (x), add (x), delete (x), or some
such or having other tables contain permission information in order to
specify access. I don't see any of this. It looks like each
permission is done at the app level and going to be hard coded against
the permission id. I think permissions need to be stated in the
database and the ideal senerio is to enforce them at this level as
well.

* I think that files should be decoupled from songs. I think there
should be a relationship to relate songs to files but I could also see
a use case to associate them with a service directly.

* I think that you should define a generic person. Thus a user
would extend a person and allow login capabilities, a preacher is a
type of person.


* If file is going to be completely stored in the database, you
might want to encoded it and state upfront how it is encoded. Some
databases handle binary data differently, sometimes you have to
encoded/decode it before storage in the database.

* Have you thought of hierachical topics?

* Shouldn't you include resources?

* Shouldn't you include locations?

* Associate locations to services?

* Bible_books seems superfluous and useless, should this be
removed?

* Should verses be grouped into passages?

* Should there be some tables for verse/passage of the day?


I am a little lost on the services; it doesn't seem very complete and
the expected relationships don't seem to be there.

It seems to be for a particular service. You might want to extend a
prototypical service. You also might want to categorize services and
associate to the service additional info such as the location,
required roles, specify people to fill those roles (and be able to
swap people). You may want to consider these roles to be not only
people but be generic resources. I would think that a service would
be broken down into a series of hierachical events, each with
different parts, verses, songs, files, collection plate, closing,
greetings, etc and associated resources at each step.

Is this what you are going to accomplish?



Thank you for your time and hope this feedback was useful.


--spt

[Raoul Snyman]

Hi,

On Nov 9, 9:42 pm, spt <s...@tangledknots.com> wrote:
> I understand that this project is still in the design phase.  I don't
> know how you feel about external feedback especially at this
> juncture.  I don't mean to step on any toes or to cause problems or to
> criticize.

I'm not going to reply to the whole e-mail right now, I don't have
time, but as you pointed out, this is still in the initial stages of
development. Yes, there are things missing, yes, there is some
cleaning up, but I haven't yet had sufficient time to iron out all the
kinks. The tables.py file you looked at was only just committed this
morning (I'm a few hours ahead of everyone else here).

You have mentioned some good points, and they are not lost on me (or
on the rest of the team, I'm sure), but as I said before, we haven't
had the time to iron out all the kinks yet, we only *just* committed
that code.

Also, Kevin is the one with the overall idea and vision. We've latched
onto his vision, but we don't know everything that he thought of
initially. I had (for instance) discussed with Joshua this morning the
possibility of adding some other fields in as well, and we decided to
start building with what we have at the moment, and add other things
as and when we see the need.

One thing I did also notice in your e-mail is that you ask about a lot
of things that should be implemented in the application logic, not in
the database. The database level is meant to take care of storing and
retrieving data, not validating e-mail addresses (for instance).

That's my 5c (South African) for now,

God bless,
Raoul.

[spt]

Oh, I guess I didn't look at actual modification times. I had assumed
it was rough, but didn't think it was quite that green ;-)

What I was talking about regarding email was that it looked to be the
username. I would think that you would want to sanitize and check
usernames somewhat stringently. I didn't recommend full email
validation but just something along the lines of length(email) > 6 and
contains a '@' char. Right now the database thinks it is acceptable
to have an email address that is blank ('').

Most of the fields that you have currently marked as not null might
also should have a constraint to check that it contains a minimal
number of chars.

Regarding the database logic, I was specifically asking questions
about permissions and how they were to be used. I didn't see how the
permission table was actually used; I had expected to see some hints
at the schema level. I believe I can make an argument why acls should
be implemented at the db level for most applications but that wasn't
my point nor intention. I was looking more along the lines of how is
access to be restricted and what methodologies, ie at tables, specific
records, by actions, etc.

My email was my own opinion of just the schema and doesn't represents
Kevin's in any form or fashion. I wasn't aware of the overall plans,
goals, or the current state of development, etc -- I just took a blind
look this morning. Kevin is very excited about this project and
couldn't say enough good things about you as individuals.

--spt

[Raoul Snyman]

On Sun, Nov 9, 2008 at 11:30 PM, spt <s...@tangledknots.com> wrote:
> What I was talking about regarding email was that it looked to be the
> username. I would think that you would want to sanitize and check
> usernames somewhat stringently. I didn't recommend full email
> validation but just something along the lines of length(email) > 6 and
> contains a '@' char. Right now the database thinks it is acceptable
> to have an email address that is blank ('').
>
> Most of the fields that you have currently marked as not null might
> also should have a constraint to check that it contains a minimal
> number of chars.

As I said before, this sort of stuff should not be in the database
level. This is application logic, and form validation. SQLAlchemy
doesn't have any way that you can build in validation, and it's quite
right not to.

> Regarding the database logic, I was specifically asking questions
> about permissions and how they were to be used. I didn't see how the
> permission table was actually used; I had expected to see some hints
> at the schema level. I believe I can make an argument why acls should
> be implemented at the db level for most applications but that wasn't
> my point nor intention. I was looking more along the lines of how is
> access to be restricted and what methodologies, ie at tables, specific
> records, by actions, etc.

The permissions system I have devised is not centered on tables and
entities, but around actions that the user can perform. This means
that we will be doing things mostly in the application logic, rather
than the db. There will be some tie-in to the tables at some point,
but I haven't worked properly on the permissions system yet. Because
we're using an MVC application framework, it's fairly simple to
implement a permissions system.

Thanks for raising those concerns though, you raise some valid issues,
and it reminds me of what we do still need to work on.

--
Raoul Snyman
B.Tech Information Technology (Software Engineering)
E-Mail: raoul....@gmail.com
Web: http://www.saturnlaboratories.co.za/
Blog: http://blog.saturnlaboratories.co.za/
Mobile: 082 550 3754
Registered Linux User #333298 (http://counter.li.org)

[spt]

I am unfamiliar with SQLAlchemy but I did take a look at this aspect
before I had initially posted. It supports UniqueConstraint,
ForeignKeyConstraint, CheckConstraint, etc style constraints provided
it is supported in the underlying database. I had also asked what
your intended database target(s) were as well.

I am a believer of you cannot have too much error checking and I
wasn't advocating to preclude the normal application level validation.

The permission and database logic wasn't the point of my email. It
was to offer feedback. I had also asked a good many questions, not to
criticize but to gain understanding.

I had mistakenly thought that the schema, code, etc had been in the
tree since this summer not only a couple of hours prior.

This seems like a closed development group and I am sorry for the line
noise. I wish you well.

--spt

> E-Mail:   raoul.sny...@gmail.com