From 33979c86030899f20e1c56ef390104436ad25f1e Mon Sep 17 00:00:00 2001 From: Bryan Date: Thu, 13 Mar 2014 03:20:54 -0400 Subject: [PATCH] Replace explicit Prof/Course model with ManyToMany relationship. --- karmaworld/apps/courses/admin.py | 2 - karmaworld/apps/courses/forms.py | 10 +- ..._del_unique_professoraffiliation_profes.py | 120 ++++++++++++++++++ karmaworld/apps/courses/models.py | 95 +++----------- karmaworld/apps/courses/views.py | 4 +- .../templates/courses/course_detail.html | 6 +- .../templates/courses/course_list_entry.html | 2 +- karmaworld/utils/forms.py | 60 ++++++++- 8 files changed, 206 insertions(+), 93 deletions(-) create mode 100644 karmaworld/apps/courses/migrations/0012_auto__del_professoraffiliation__del_unique_professoraffiliation_profes.py diff --git a/karmaworld/apps/courses/admin.py b/karmaworld/apps/courses/admin.py index 7e56d10..bfafca1 100644 --- a/karmaworld/apps/courses/admin.py +++ b/karmaworld/apps/courses/admin.py @@ -11,7 +11,6 @@ from karmaworld.apps.courses.models import School from karmaworld.apps.courses.models import Course from karmaworld.apps.courses.models import Professor from karmaworld.apps.courses.models import Department -from karmaworld.apps.courses.models import ProfessorAffiliation class CourseAdmin(reversion.VersionAdmin, admin.ModelAdmin): """ an Admin handler for the Course model that handles fk search """ @@ -31,4 +30,3 @@ admin.site.register(School) admin.site.register(Professor) admin.site.register(Course, CourseAdmin) admin.site.register(Department, DepartmentAdmin) -admin.site.register(ProfessorAffiliation) diff --git a/karmaworld/apps/courses/forms.py b/karmaworld/apps/courses/forms.py index 66af081..d898f7f 100644 --- a/karmaworld/apps/courses/forms.py +++ b/karmaworld/apps/courses/forms.py @@ -57,7 +57,7 @@ class ProfessorForm(NiceErrorModelForm, ACFieldModelForm): In practice, better safe than incoherent. """ oldprof = None - if hasattr(self, 'instance') and self.instance: + if hasattr(self, 'instance') and self.instance and self.instance.pk: # Professor was already autocompleted. Save that object. oldprof = self.instance # Extract the field value, possibly replacing self.instance @@ -128,8 +128,12 @@ class CourseForm(NiceErrorModelForm, DependentModelForm): model = Course # order the fields fields = ('name', 'url') - # pass department data onto DepartmentForm - model_fields = {'department': DepartmentForm} + model_fields = { + # pass department data onto DepartmentForm + 'department': DepartmentForm, + # pass professor data onto ProfessorForm + 'professor': ProfessorForm, + } def clean(self, *args, **kwargs): """ Honeypot validation. """ diff --git a/karmaworld/apps/courses/migrations/0012_auto__del_professoraffiliation__del_unique_professoraffiliation_profes.py b/karmaworld/apps/courses/migrations/0012_auto__del_professoraffiliation__del_unique_professoraffiliation_profes.py new file mode 100644 index 0000000..8bddfc3 --- /dev/null +++ b/karmaworld/apps/courses/migrations/0012_auto__del_professoraffiliation__del_unique_professoraffiliation_profes.py @@ -0,0 +1,120 @@ +# -*- coding: utf-8 -*- +from south.utils import datetime_utils as datetime +from south.db import db +from south.v2 import SchemaMigration +from django.db import models + + +class Migration(SchemaMigration): + + def forwards(self, orm): + # Removing unique constraint on 'Course', fields ['name', 'department'] + db.delete_unique(u'courses_course', ['name', 'department_id']) + + # Removing unique constraint on 'ProfessorTaught', fields ['professor', 'course'] + db.delete_unique(u'courses_professortaught', ['professor_id', 'course_id']) + + # Removing unique constraint on 'ProfessorAffiliation', fields ['professor', 'department'] + db.delete_unique(u'courses_professoraffiliation', ['professor_id', 'department_id']) + + # Deleting model 'ProfessorAffiliation' + db.delete_table(u'courses_professoraffiliation') + + # Deleting model 'ProfessorTaught' + db.delete_table(u'courses_professortaught') + + # Adding M2M table for field professor on 'Course' + m2m_table_name = db.shorten_name(u'courses_course_professor') + db.create_table(m2m_table_name, ( + ('id', models.AutoField(verbose_name='ID', primary_key=True, auto_created=True)), + ('course', models.ForeignKey(orm[u'courses.course'], null=False)), + ('professor', models.ForeignKey(orm[u'courses.professor'], null=False)) + )) + db.create_unique(m2m_table_name, ['course_id', 'professor_id']) + + # Adding unique constraint on 'Course', fields ['name', 'school'] + db.create_unique(u'courses_course', ['name', 'school_id']) + + + def backwards(self, orm): + # Removing unique constraint on 'Course', fields ['name', 'school'] + db.delete_unique(u'courses_course', ['name', 'school_id']) + + # Adding model 'ProfessorAffiliation' + db.create_table(u'courses_professoraffiliation', ( + ('department', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['courses.Department'])), + ('professor', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['courses.Professor'])), + (u'id', self.gf('django.db.models.fields.AutoField')(primary_key=True)), + )) + db.send_create_signal(u'courses', ['ProfessorAffiliation']) + + # Adding unique constraint on 'ProfessorAffiliation', fields ['professor', 'department'] + db.create_unique(u'courses_professoraffiliation', ['professor_id', 'department_id']) + + # Adding model 'ProfessorTaught' + db.create_table(u'courses_professortaught', ( + ('course', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['courses.Course'])), + ('professor', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['courses.Professor'])), + (u'id', self.gf('django.db.models.fields.AutoField')(primary_key=True)), + )) + db.send_create_signal(u'courses', ['ProfessorTaught']) + + # Adding unique constraint on 'ProfessorTaught', fields ['professor', 'course'] + db.create_unique(u'courses_professortaught', ['professor_id', 'course_id']) + + # Removing M2M table for field professor on 'Course' + db.delete_table(db.shorten_name(u'courses_course_professor')) + + # Adding unique constraint on 'Course', fields ['name', 'department'] + db.create_unique(u'courses_course', ['name', 'department_id']) + + + models = { + u'courses.course': { + 'Meta': {'ordering': "['-file_count', 'school', 'name']", 'unique_together': "(('name', 'school'),)", 'object_name': 'Course'}, + 'created_at': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), + 'department': ('django.db.models.fields.related.ForeignKey', [], {'to': u"orm['courses.Department']", 'null': 'True', 'blank': 'True'}), + 'desc': ('django.db.models.fields.TextField', [], {'max_length': '511', 'null': 'True', 'blank': 'True'}), + 'file_count': ('django.db.models.fields.IntegerField', [], {'default': '0'}), + 'flags': ('django.db.models.fields.IntegerField', [], {'default': '0'}), + u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'instructor_email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'null': 'True', 'blank': 'True'}), + 'instructor_name': ('django.db.models.fields.CharField', [], {'max_length': '255', 'null': 'True', 'blank': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '255'}), + 'professor': ('django.db.models.fields.related.ManyToManyField', [], {'symmetrical': 'False', 'to': u"orm['courses.Professor']", 'null': 'True', 'blank': 'True'}), + 'school': ('django.db.models.fields.related.ForeignKey', [], {'to': u"orm['courses.School']", 'null': 'True', 'blank': 'True'}), + 'slug': ('django.db.models.fields.SlugField', [], {'max_length': '150', 'null': 'True'}), + 'updated_at': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.utcnow'}), + 'url': ('django.db.models.fields.URLField', [], {'max_length': '511', 'null': 'True', 'blank': 'True'}) + }, + u'courses.department': { + 'Meta': {'unique_together': "(('name', 'school'),)", 'object_name': 'Department'}, + u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '255'}), + 'school': ('django.db.models.fields.related.ForeignKey', [], {'to': u"orm['courses.School']"}), + 'slug': ('django.db.models.fields.SlugField', [], {'max_length': '150', 'null': 'True'}), + 'url': ('django.db.models.fields.URLField', [], {'max_length': '511', 'null': 'True', 'blank': 'True'}) + }, + u'courses.professor': { + 'Meta': {'unique_together': "(('name', 'email'),)", 'object_name': 'Professor'}, + 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'null': 'True', 'blank': 'True'}), + u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '255'}) + }, + u'courses.school': { + 'Meta': {'ordering': "['-file_count', '-priority', 'name']", 'object_name': 'School'}, + 'alias': ('django.db.models.fields.CharField', [], {'max_length': '255', 'null': 'True', 'blank': 'True'}), + 'facebook_id': ('django.db.models.fields.BigIntegerField', [], {'null': 'True', 'blank': 'True'}), + 'file_count': ('django.db.models.fields.IntegerField', [], {'default': '0'}), + 'hashtag': ('django.db.models.fields.CharField', [], {'max_length': '16', 'unique': 'True', 'null': 'True', 'blank': 'True'}), + u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'location': ('django.db.models.fields.CharField', [], {'max_length': '255', 'null': 'True', 'blank': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '255'}), + 'priority': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'slug': ('django.db.models.fields.SlugField', [], {'max_length': '150', 'null': 'True'}), + 'url': ('django.db.models.fields.URLField', [], {'max_length': '511', 'blank': 'True'}), + 'usde_id': ('django.db.models.fields.BigIntegerField', [], {'unique': 'True', 'null': 'True', 'blank': 'True'}) + } + } + + complete_apps = ['courses'] \ No newline at end of file diff --git a/karmaworld/apps/courses/models.py b/karmaworld/apps/courses/models.py index ab21977..7b3c48d 100644 --- a/karmaworld/apps/courses/models.py +++ b/karmaworld/apps/courses/models.py @@ -225,43 +225,6 @@ class ProfessorEmailLookup(FieldLookupChannel): field_lookup = 'email' -class ProfessorAffiliationManager(models.Manager): - """ Handle restoring data. """ - def get_by_natural_key(self, prof, dept): - """ - Return a ProfessorAffiliation defined by prof and department. - """ - return self.get(professor=prof,department=dept) - - -class ProfessorAffiliation(models.Model): - """ - Track professors for departments. (many-to-many) - """ - objects = ProfessorAffiliationManager() - - professor = models.ForeignKey(Professor) - department = models.ForeignKey(Department) - - def __unicode__(self): - return u'Professor {0} working for {1}'.format(unicode(self.professor), unicode(self.department)) - - class Meta: - """ - Many-to-many across both professor and department. - However, (prof, dept) as a tuple should only appear once. - """ - unique_together = ('professor', 'department',) - - def natural_key(self): - """ - A ProfessorAffiliation is uniquely defined by the prof and department - """ - return (self.professor.natural_key(), self.department.natural_key()) - # Requires dependencies to be dumped first - natural_key.dependencies = ['courses.professor','courses.department'] - - class CourseManager(models.Manager): """ Handle restoring data. """ def get_by_natural_key(self, name, dept): @@ -289,7 +252,9 @@ class Course(models.Model): url = models.URLField(max_length=511, blank=True, null=True, verbose_name="Course URL") - # instructor_* is vestigial, replaced by Professor+ProfessorTaught models. + # professor should remove nullable when school instructor_* yoinked + professor = models.ManyToManyField(Professor, blank=True, null=True) + # instructor_* is vestigial instructor_name = models.CharField(max_length=255, blank=True, null=True) instructor_email = models.EmailField(blank=True, null=True) @@ -354,6 +319,22 @@ class Course(models.Model): # The value might be None, return zero in that case with shortcut logic. return self.note_set.aggregate(x=models.Sum('thanks'))['x'] or 0 + def get_prof_names(self): + """ Comma separated list of professor names. """ + # old style: just use the given name + if self.instructor_name: + return str(self.instructor_name) + # Run through all associated professors and concatenate their names. + return ','.join(self.professor.values_list('name', flat=True)) + + def get_prof_emails(self): + """ Comma separated list of professor emails. """ + # old style: just use the given name + if self.instructor_email: + return str(self.instructor_email) + # Run through all associated professors and concatenate their names. + return ','.join(self.professor.values_list('email', flat=True)) + reversion.register(Course) @@ -374,45 +355,9 @@ class CourseNameLookup(FieldLookupChannel): return results.values_list(self.field_lookup, flat=True) -class ProfessorTaughtManager(models.Manager): - """ Handle restoring data. """ - def get_by_natural_key(self, prof, course): - """ - Return a ProfessorTaught defined by professor and course. - """ - return self.get(professor=prof, course=course) - - -class ProfessorTaught(models.Model): - """ - Track professors teaching courses. (many-to-many) - """ - objects = ProfessorTaughtManager() - - professor = models.ForeignKey(Professor) - course = models.ForeignKey(Course) - - def __unicode__(self): - return u'Professor {0} taught {1}'.format(unicode(self.professor), unicode(self.course)) - - class Meta: - # many-to-many across both fields, - # but (prof, course) as a tuple should only appear once. - unique_together = ('professor', 'course',) - - def natural_key(self): - """ - A ProfessorTaught is uniquely defined by the prof and course. - """ - return (self.professor.natural_key(), self.course.natural_key()) - # Requires dependencies to be dumped first - natural_key.dependencies = ['courses.professor','courses.course'] - - # Enforce unique constraints even when we're using a database like # SQLite that doesn't understand them auto_add_check_unique_together(Course) +auto_add_check_unique_together(School) auto_add_check_unique_together(Department) auto_add_check_unique_together(Professor) -auto_add_check_unique_together(ProfessorAffiliation) -auto_add_check_unique_together(ProfessorTaught) diff --git a/karmaworld/apps/courses/views.py b/karmaworld/apps/courses/views.py index 6d2b814..30dcb77 100644 --- a/karmaworld/apps/courses/views.py +++ b/karmaworld/apps/courses/views.py @@ -18,14 +18,12 @@ from django.views.generic.edit import CreateView from karmaworld.apps.courses.models import Course from karmaworld.apps.courses.models import School +from karmaworld.apps.courses.forms import CourseForm from karmaworld.apps.notes.models import Note from karmaworld.apps.users.models import CourseKarmaEvent from karmaworld.apps.notes.forms import FileUploadForm from karmaworld.utils import ajax_increment, format_session_increment_field -# ProfessorTaughtForm contains CourseForm -from karmaworld.apps.courses.forms import ProfessorTaughtForm as CourseForm - FLAG_FIELD = 'flags' USER_PROFILE_FLAGS_FIELD = 'flagged_courses' diff --git a/karmaworld/templates/courses/course_detail.html b/karmaworld/templates/courses/course_detail.html index 8a731da..6def87e 100644 --- a/karmaworld/templates/courses/course_detail.html +++ b/karmaworld/templates/courses/course_detail.html @@ -41,7 +41,7 @@
- {{ course.instructor_name }} + {{ course.get_prof_names }} {% if course.department %}// {{ course.department.name }}{% endif %}
@@ -134,12 +134,12 @@
Instructor Name: - +
Instructor Email: - +
diff --git a/karmaworld/templates/courses/course_list_entry.html b/karmaworld/templates/courses/course_list_entry.html index 48dd1da..985bb85 100644 --- a/karmaworld/templates/courses/course_list_entry.html +++ b/karmaworld/templates/courses/course_list_entry.html @@ -22,5 +22,5 @@
{% if course.school %}{{ course.school.name }}{% else %}{{ course.department.school.name }}{% endif %} - {{ course.instructor_name }} + {{ course.get_prof_names }} diff --git a/karmaworld/utils/forms.py b/karmaworld/utils/forms.py index 296ed2a..4025870 100644 --- a/karmaworld/utils/forms.py +++ b/karmaworld/utils/forms.py @@ -10,6 +10,13 @@ from django.forms.util import ErrorList from django.utils.safestring import mark_safe +# helper function to determine if field of form is ManyToMany. +def is_m2m(form, field): + modelfield = getattr(form.Meta.model, field) + # check by contract (quacks like a duck) rather than instance checking + return hasattr(modelfield, 'through') and hasattr(modelfield, 'field') + + # Django hard codes CSS attributes into ModelForm returned ErrorList # https://github.com/django/django/blob/1.5.5/django/forms/util.py#L54-L60 # https://docs.djangoproject.com/en/1.5/ref/forms/api/#customizing-the-error-list-format @@ -94,8 +101,11 @@ class DependentModelForm(ModelForm): self.dependent_modelforms_data = {} super(DependentModelForm, self).__init__(*args, **kwargs) - def _get_forms(self, with_data=False): + def _get_forms(self): """ Memoize dependent form objects. """ + # Determine if there is data. + with_data = True if self.data else False + # Memoize data separately if there is or is not data. data = (self.data,) if with_data else tuple() memo = self.dependent_modelforms_data if with_data else \ self.dependent_modelforms @@ -138,7 +148,7 @@ class DependentModelForm(ModelForm): """ Check all subforms for validity and then this form. """ all_valid = True # Perform validation and error checking for each ModelForm. - for attribute, modelform in self._get_forms(with_data=True).iteritems(): + for attribute, modelform in self._get_forms().iteritems(): if not modelform.is_valid(): all_valid = False @@ -148,7 +158,7 @@ class DependentModelForm(ModelForm): def _post_clean(self, *args, **kwargs): """ Inject objects created from required ModelForms. """ - super(ModelForm, self)._post_clean(*args, **kwargs) + super(DependentModelForm, self)._post_clean(*args, **kwargs) # If self.instance has not been created by _post_clean, create it now. # This happens when only model_fields are present and no fields. @@ -161,9 +171,47 @@ class DependentModelForm(ModelForm): if not self.is_valid(): return - for attribute, modelform in self._get_forms(with_data=True).iteritems(): - # create foreign model object and associate it internally here - setattr(self.instance, attribute, modelform.save()) + # create foreign model object and associate it internally here + for attribute, modelform in self._get_forms().iteritems(): + # handle ManyToMany versus ForeignKey + if not is_m2m(self, attribute): + # Handle Foreign Key, assign object directly to attribute + setattr(self.instance, attribute, modelform.save()) + + def save(self, *args, **kwargs): + """ Handle ManyToMany objects. """ + instance = super(DependentModelForm, self).save(*args, **kwargs) + + # Check for and update any M2M model_fields + if hasattr(self.instance, 'pk') and self.instance.pk: + # objects were created during commit. associate M2M now. + for attribute, modelform in self._get_forms().iteritems(): + # handle ManyToMany versus ForeignKey + if is_m2m(self, attribute): + # use add() to create association between models. + getattr(instance, attribute).add(modelform.save()) + else: + # objects not yet in database. save for later. + # reference to save_m2m deferred function from BaseModelForm.save() + old_save_m2m = self.save_m2m + + # write a new save_m2m with Python closure power! + def save_m2m(): + # call "super" + old_save_m2m() + # associate M2M objects with self instance + for attribute, modelform in self._get_forms().iteritems(): + # ManyToMany contract test. + # use add() to create association between models. + objattr = getattr(instance, attribute, None) + if objattr and hasattr(objattr, 'add'): + objattr.add(modelform.save()) + + # save function for later just as BaseModelForm.save() does + self.save_m2m = save_m2m + + # passthrough instance + return instance def _render_dependencies_first(self, method, *args, **kwargs): """ Render dependent forms prior to rendering this form. """ -- 2.25.1