Review my Django Model - Need lots of suggestions.

Question

Hi all,

I am pulling a variety of information sources to build up a profile of a person. Once I do this I want to the flexibility to look at a person in a different ways. I don't have a lot of expierience in django so I would like a critique (be gentle) of my model. Admittedly even as I coded this I'm thinking redundancy (against DRY) but I would like to know what others think. FWIW - this data was pulled not created - so maybe I shouldn't keep it all but dropping data seems bad..

Specifically I want to know if the use of ManyToMany is appropriate or would you just KISS and leave each entry as it's own without any ManyToMany business. Long term I think KISS would make updating it simple (a basic for loop) but is there a tradeoff with being able to query a person (say based on location_description or job_function). Anyway I would appreciate some input.

class Person(models.Model):

    """This simply builds a notes user"""

    aliases = models.ManyToManyField(Aliases)  #Hmm this is list..
    assistant = models.CharField(max_length = 255, blank = True)
    cell_phone = models.CharField(max_lenth = 16, blank = True)
    city = models.ManyToManyField(City)
    country = models.ManyToManyField(County)
    department = models.ManyToManyField(Department)
    departmeht_number = models.ManyToManyField(Department_Number)
    division_code = models.ManyToManyField(Division_Code)
    email = models.EmailField(max_length = 50)
    functional_area = models.ManyToManyField(Functional_Area)
    # jpeg = models.
    job_classification = models.ManyToManyField(Job_Classification)
    job_classification_code = models.ManyToManyField(Job_Classification_Code)
    last_name = models.CharField(max_length = 255)
    location_description = models.ManyToManyField(Location_Description)
    location_path = models.ManyToManyField(Location_Path)
    mail_address = models.CharField(max_length = 255, blank = True)
    mail_domain = models.ManyToManyField(Mail_Domain)
    mail_file = models.CharField(max_length = 255, blank = True)
    mail_server = models.ManyToManyField(Mail_Server)
    match_pct = models.CharField(max_lenth = 6)
    name = models.CharField(max_length = 255)
    name_reverse = models.CharField(max_length = 255)
    nickname = models.CharField(max_length = 32)
    notes_url = models.URLField()
    #    object_class = models.
    office_phone = models.CharField(max_length = 255, blank = True)
    other_phone = models.CharField(max_length = 255, blank = True)
    position = models.ManyToManyField(Position)
    section = models.ManyToManyField(Section)
    section_code = models.ManyToManyField(SectionCode)
    shift = models.ManyToManyField(Shift)
    state = models.ManyToManyFiedl(State)
    supervisor = models.ManyToManyField(Supervisor)
    supervisor_reverse = models.ManyToManyField(Supervisor_reverse)
    uid = models.CharField(max_length = 128)

    def __unicode__(self):
        return str(self.name)

Answer 1

It seems to me that many of your ManyToMany relations should be replaced by properties of the target model.

For example, you've got city and country as separate relations. That means that you might end up with a user having city = ['Paris', 'London', 'New York'] and country = ['USA', 'France', 'UK'].

Instead I think you should set up a locations ManyToMany relation pointing to a model consisting of city and country properties.

Same with department and department_number, job_classification and job_classification_code, location_description and location_path, etc.

Edit: In some cases you might be able to use ForeignKey instead of ManyToMany. Perhaps a user can only belong to one city and country?

Position and supervisor will likely be some of the trickier relations, because position should point to a department/unit and is_supervisor should likely be a property of a position. That is, you only need to point to a position as the supervisor(s) would be retrieved from other users' positions.

Edit: Example: If person A works in dept X and person B also works in dept X, but as a manager, then person A is supervised by person B.

Edit: It can be tempting to assume that a department can only have one manager or that a manager can only manage one department. I would advice against this as organizations often have all kinds of special cases. Think about scenarios like when a manager leaves the company, will the department be without manager for a while or will another manager temporarily take over, thus keeping two manager positions.

Answer 2

I'd make subordinate field and remove supervisor.

 subordinates = models.ManyToManyField('self', related_name='supervisors', through=Position)

This way supervisors will be an automatic reverse lookup field and there will be a record of Position that connects supervisor with the subordinate

edit: actually I'd also suggest to carefully think of re-designing your models. Maybe you won't need as many as you suggest.

Looks like you have "department", "company" (if you need to track multiple companies), "position", "location" and "person"... Decide what big entities to keep, then think of where exactly each field belongs and how to link up the "big" objects that will translate to tables.

It will probably be best to make subordinate-supervisor relation between Position objects, because people come and go, but positon are more permanent. So I'd have:

class Position:
    title = models.CharField() #product manager, engineer, etc.
    employee = models.ForeignKey(Person, related_name='positions')
    subordinates = models.ManyToManyField('self', related_name='supervisors')

of if you don't need multiple supervisors per position, use

    supervisor = models.ForeignKey('self', related_name='subordinates')

Even though now there is not subordiantes field, it will be accessible as previously through related name.

Where does location belong?

If you decide that both Person and Department must have location, then you might think of Generic relations, which are albeit not as easy to work with as "regular ones", but you can also get away with adding "type" to location ('private/business') and then use either Location.company or Location.residence_owner and avoid using generic relations.

Answer 3

Here's a few thoughts.

I think you should create separate models for department and job_classification and so on. For example

class department(models.Model):
    name = Models.CharField(max_length=100)
    number = Models.IntegerField

and then have a ManyToManyField to the department. Otherwise, if you have a person with department = ['HR', 'Finance'] and department_number = [12, 5], I don't think there's any way to tell which department number represents which department.

I would find the model easier to read if the fields were grouped by what they describe, rather than ordered alphabetically. E.G I think name, nickname, last_name should be defined together.

Finally, perhaps Alias should be a seperate model, with a ForeignKey to Person, since only one person should have any particular alias.

Answer 4

__unicode__ should return unicode

def __unicode__(self):
       return u'%s' %self.name

Django provides a emailfield for complete adresses:

mail = models.EmailField()

I think, a address model might be sense-full. A person could have several addresses (work, home,...)

edit

I just saw, you are using emailfield.

What is this for:

mail_address = models.CharField(max_length = 255, blank = True)
mail_domain = models.ManyToManyField(Mail_Domain)
mail_file = models.CharField(max_length = 255, blank = True)
mail_server = models.ManyToManyField(Mail_Server)

?

edit

You dont have to use ManyToManyFields all the time. Check out ForeignKey, OneToOneField and the generic framework

Answer 5

I would also suggest that you use a separate model for employment. Use a foreign key back to your Person object. You can add as many records to that table as necessary.

Same suggestion goes for the mail_* fields as well.