Music info model

Question

I'm learning Django as I go. I know this model is missing user authentication, registration, comments/comment threading, and voting. But this is my starting code for my model. What are some of the things I can improve on, modify, rewrite, etc?

from django.contrib.auth.models import User
from django.db import models
from django.contrib import admin
from django.template.defaultfilters import escape
from django.utils.translation import ugettext as _
from django.utils.encoding import force_unicode
from django.http import HttpResponse, HttpResponseRedirect
from django.core.urlresolvers import reverse

class DateTime(models.Model):
  datetime = models.DateTimeField(auto_now_add=True)

  def __unicode__(self):
    return unicode(self.datetime.strftime("%b %d, %Y, %I:%M %p"))

class Country(models.Model):
  country = models.CharField(max_length=50)

  def __unicode__(self):
    return unicode(self.country)

class Artist(models.Model):
  artist = models.CharField(max_length=50)
  country = models.ForeignKey(Country, blank=True, null=True)
  user = models.ForeignKey(User, blank=True, null=True)
  created = models.ForeignKey(DateTime)
  notes = models.TextField()

  def __unicode__(self):
    return artist

class Song(models.Model):
  name = models.CharField(max_length=200)
  artist = models.ForeignKey(Artist, blank=True, null=True)
  # language = models.ForeignKey(Country, blank=True, null=True)
  user = models.ForeignKey(User, blank=True, null=True)
  created = models.ForeignKey(DateTime)
  notes = models.TextField()

  def __unicode__(self):
    return song

class FileType(models.Model):
  file_type = models.CharField(max_length=3)
  description = models.TextField()
  user = models.ForeignKey(User, blank=True, null=True)
  created = models.ForeignKey(DateTime)
  notes = models.TextField()

  def __unicode__(self):
    return file_type

class Level(models.Model):
  level = models.CharField(max_length=3)
  description = models.TextField()
  user = models.ForeignKey(User, blank=True, null=True)
  created = models.ForeignKey(DateTime)
  notes = models.TextField()

  def __unicode__(self):
    return level

class MusicSheet(models.Model):
  version = models.CharField(max_length=2)
  song = models.ForeignKey(Song, blank=True, null=True)
  artist = models.ForeignKey(Artist, blank=True, null=True)
  file_type = models.ForeignKey(FileType, blank=True, null=True)
  level = models.ForeignKey(Level, blank=True, null=True)
  user = models.ForeignKey(User, blank=True, null=True)
  created = models.ForeignKey(DateTime)
  text = models.TextField()
  notes = models.TextField()

#include votes

#include comments

#include registration code


########################################################################################################################
######################################## ADMIN STUFF ###################################################################
########################################################################################################################


class MusicSheetAdmin(admin.ModelAdmin):
  list_display = ["version", "song", "artist", "file_type", "level", "user" , "created", "text", "notes"]
  search_fields = ["version"]

class MusicSheetInline(admin.TabularInline):
  model = MusicSheet

class DateAdmin(admin.ModelAdmin):
  list_display = ["datetime"]
  inlines = [MusicSheetInline]

  def response_add(self, request, obj, post_url_continue='../%s/'):
    """ Determines the HttpResponse for the add_view stage.  """
    opts = obj._meta
    pk_value = obj._get_pk_val()

    msg = "Song(s) were added successfully."
    # Here, we distinguish between different save types by checking for
    # the presence of keys in request.POST.
    if request.POST.has_key("_continue"):
      self.message_user(request, msg + ' ' + _("You may edit it again below."))
      if request.POST.has_key("_popup"):
        post_url_continue += "?_popup=1"
      return HttpResponseRedirect(post_url_continue % pk_value)

    if request.POST.has_key("_popup"):
      return HttpResponse(
          '<script type="text/javascript">opener.dismissAddAnotherPopup(window, "%s", "%s");'
          '</script>' % (escape(pk_value), escape(obj)))
    elif request.POST.has_key("_addanother"):
      self.message_user(request, msg + ' ' + (_("You may add another %s below.") %
                                              force_unicode(opts.verbose_name)))
      return HttpResponseRedirect(request.path)
    else:
      self.message_user(request, msg)

    for music_sheet in MusicSheet.objects.filter(created=obj):
      if not music_sheet.user:
        music_sheet.user = request.user
        music_sheet.save()

    return HttpResponseRedirect(reverse("admin:musicsheet_musicsheet_changelist"))


class CountryAdmin(admin.ModelAdmin):
  list_display = ["country"]
  search_fields = ["country"]

class CountryInline(admin.TabularInline):
  model = MusicSheet



class SongAdmin(admin.ModelAdmin):
  list_display = ["name", "artist", "user", "created", "notes"]
  search_fields = ["name", "artist"]

class SongInline(admin.TabularInline):
  model = MusicSheet



class ArtistAdmin(admin.ModelAdmin):
  list_display = ["artist", "country", "user", "created", "notes"]
  search_fields = ["artist", "country"]

class ArtistInline(admin.TabularInline):
  model = MusicSheet



class FileTypeAdmin(admin.ModelAdmin):
  list_display = ["file_type", "description", "user", "created", "notes"]
  search_fields = ["file_type"]

class FileTypeInline(admin.StackedInline):
  model = MusicSheet


class LevelAdmin(admin.ModelAdmin):
  list_display = ["level", "description", "user", "created", "notes"]
  search_fields = ["level"]

class LevelInline(admin.StackedInline):
  model = MusicSheet

admin.site.register(MusicSheet, MusicSheetAdmin)
admin.site.register(Country, CountryAdmin)
admin.site.register(Song, SongAdmin)
admin.site.register(Artist, ArtistAdmin)
admin.site.register(FileType, FileTypeAdmin)
admin.site.register(Level, LevelAdmin)
admin.site.register(DateTime, DateAdmin)

Answer 1

Disclaimer: I'm not a Django guru. Here are my thoughts, for what it's worth.

• Common fields: user, created, and nodes occur in almost every class. I'd consider creating an abstract base class with these three fields.

• Field names: for the sake of readability, I'd change Artist.artist to Artist.name, and *.user to *.creator.

• File extensions: occasionally have more than 3 chars (e.g.: .jpeg, .java).

• Indexing: I'd add db_index=True to the name fields, for the very least.

• Redundancy: since you have MusicSheet.song and Song.artist, you no longer need MusicSheet.artist. It's called redundancy and it's generally a bad thing.

• Primary keys, unique constraints: you probably don't want two FileTypes with the same file_type, so that field is a good candidate for primary key (primary_key=True). I'm not sure what Level is supposed to model, but my guess is that its level is a good candidate for primary key as well. Add unique=True for Country.country, or add a primary key with country code.

• DateTime: I don't see a need for this class. Simply replace every occurrence of: created = models.ForeignKey(DateTime) with models.DateTimeField(auto_now_add=True).

Answer 2

A few things come to mind:

• Lose the DateTime model and instead add a DateTimeField directly to the models that need it. The ForeignKey buys you nothing except incuring an additional database join for even the simplest of operations.
• You probably did this only so you could paste this code in one piece, but if not: You should put your admin code in a file called admin.py inside your application. That way you can use the admin autodiscover feature
• In DateAdmin you overwrite the response_add method to (as far as I can see) set the user/creator field. This can be acomplished much easier by overwriting the save_model method.

Answer 3

• Instead of the (very long) list of Admin classes at the end, you can integrate them in the model, so:

  class MusicSheet(models.Model):
      version = models.CharField(max_length=2)
      song = models.ForeignKey(Song, blank=True, null=True)
      artist = models.ForeignKey(Artist, blank=True, null=True)
      file_type = models.ForeignKey(FileType, blank=True, null=True)
      level = models.ForeignKey(Level, blank=True, null=True)
      user = models.ForeignKey(User, blank=True, null=True)
      created = models.ForeignKey(DateTime)
      text = models.TextField()
      notes = models.TextField()
  
  class MusicSheetAdmin(admin.ModelAdmin):
      list_display = ["version", "song", "artist", "file_type", "level", "user" , "created", "text", "notes"]
      search_fields = ["version"]
  
  class MusicSheetInline(admin.TabularInline):
      model = MusicSheet
  
  admin.site.register(MusicSheet, MusicSheetAdmin)
  

  can be simplified (and neatened) to:

  class MusicSheet(models.Model):
      version = models.CharField(max_length=2)
      song = models.ForeignKey(Song, blank=True, null=True)
      artist = models.ForeignKey(Artist, blank=True, null=True)
      file_type = models.ForeignKey(FileType, blank=True, null=True)
      level = models.ForeignKey(Level, blank=True, null=True)
      user = models.ForeignKey(User, blank=True, null=True)
      created = models.ForeignKey(DateTime)
      text = models.TextField()
      notes = models.TextField()
  
      class Admin:
          list_display = ["version", "song", "artist", "file_type", "level", "user" , "created", "text", "notes"]
          search_fields = ["version"]
  

  Make sure that class Admin is inside the main class.

  Alternatively, if you did want them separate, the admin classes and registrations belong inside a separate file, admin.py. (You will have to import the models from myapp.models import *)

• Some of your ForeignKeys have both blank=True and null=True. This is generally not a good idea. Do you want to allow them to be null (no value) or blank (an empty value)? Looking at your code, I'd say you want null=True but I can't be sure. Try one, then the other, and see which one does what you want. I highly doubt you need both. As a general rule, I only use blank=True for CharFields, so you can have a null string instead of an empty database field.

• I can't work out what DateAdmin is meant to do. You probably won't need it when you switch to DateTimeField as recommended above, but I would want to know what its intended purpose is before I make a call.