diff --git a/lms/djangoapps/courseware/tests/test_course_survey.py b/lms/djangoapps/courseware/tests/test_course_survey.py index 7fa1960f33..6094d06c7f 100644 --- a/lms/djangoapps/courseware/tests/test_course_survey.py +++ b/lms/djangoapps/courseware/tests/test_course_survey.py @@ -4,10 +4,12 @@ Python tests for the Survey workflows from collections import OrderedDict from nose.plugins.attrib import attr +from copy import deepcopy from django.core.urlresolvers import reverse +from django.contrib.auth.models import User -from survey.models import SurveyForm +from survey.models import SurveyForm, SurveyAnswer from common.test.utils import XssTestMixin from xmodule.modulestore.tests.factories import CourseFactory @@ -65,6 +67,8 @@ class SurveyViewsTests(LoginEnrollmentTestCase, ModuleStoreTestCase, XssTestMixi self.enroll(self.course_without_survey, True) self.enroll(self.course_with_bogus_survey, True) + self.user = User.objects.get(email=email) + self.view_url = reverse('view_survey', args=[self.test_survey_name]) self.postback_url = reverse('submit_answers', args=[self.test_survey_name]) @@ -137,6 +141,52 @@ class SurveyViewsTests(LoginEnrollmentTestCase, ModuleStoreTestCase, XssTestMixi self._assert_no_redirect(self.course) + def test_course_id_field(self): + """ + Assert that the course_id will be in the form fields, if available + """ + + resp = self.client.get( + reverse( + 'course_survey', + kwargs={'course_id': unicode(self.course.id)} + ) + ) + + self.assertEqual(resp.status_code, 200) + expected = ''.format( + course_id=unicode(self.course.id) + ) + + self.assertContains(resp, expected) + + def test_course_id_persists(self): + """ + Assert that a posted back course_id is stored in the database + """ + + answers = deepcopy(self.student_answers) + answers.update({ + 'course_id': unicode(self.course.id) + }) + + resp = self.client.post( + self.postback_url, + answers + ) + self.assertEquals(resp.status_code, 200) + + self._assert_no_redirect(self.course) + + # however we want to make sure we persist the course_id + answer_objs = SurveyAnswer.objects.filter( + user=self.user, + form=self.survey + ) + + for answer_obj in answer_objs: + self.assertEquals(answer_obj.course_key, self.course.id) + def test_visiting_course_with_bogus_survey(self): """ Verifies that going to the courseware with a required, but non-existing survey, does not redirect diff --git a/lms/djangoapps/survey/migrations/0002_auto__add_field_surveyanswer_course_key.py b/lms/djangoapps/survey/migrations/0002_auto__add_field_surveyanswer_course_key.py new file mode 100644 index 0000000000..28ad94775e --- /dev/null +++ b/lms/djangoapps/survey/migrations/0002_auto__add_field_surveyanswer_course_key.py @@ -0,0 +1,80 @@ +# -*- 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): + # Adding field 'SurveyAnswer.course_key' + db.add_column('survey_surveyanswer', 'course_key', + self.gf('xmodule_django.models.CourseKeyField')(max_length=255, null=True, db_index=True), + keep_default=False) + + + def backwards(self, orm): + # Deleting field 'SurveyAnswer.course_key' + db.delete_column('survey_surveyanswer', 'course_key') + + + models = { + 'auth.group': { + 'Meta': {'object_name': 'Group'}, + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}), + 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}) + }, + 'auth.permission': { + 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'}, + 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'}) + }, + 'auth.user': { + 'Meta': {'object_name': 'User'}, + 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}), + 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}), + 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}), + 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}) + }, + 'contenttypes.contenttype': { + 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"}, + 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'}) + }, + 'survey.surveyanswer': { + 'Meta': {'object_name': 'SurveyAnswer'}, + 'course_key': ('xmodule_django.models.CourseKeyField', [], {'max_length': '255', 'null': 'True', 'db_index': 'True'}), + 'created': ('model_utils.fields.AutoCreatedField', [], {'default': 'datetime.datetime.now'}), + 'field_name': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}), + 'field_value': ('django.db.models.fields.CharField', [], {'max_length': '1024'}), + 'form': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['survey.SurveyForm']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'modified': ('model_utils.fields.AutoLastModifiedField', [], {'default': 'datetime.datetime.now'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"}) + }, + 'survey.surveyform': { + 'Meta': {'object_name': 'SurveyForm'}, + 'created': ('model_utils.fields.AutoCreatedField', [], {'default': 'datetime.datetime.now'}), + 'form': ('django.db.models.fields.TextField', [], {}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'modified': ('model_utils.fields.AutoLastModifiedField', [], {'default': 'datetime.datetime.now'}), + 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '255', 'db_index': 'True'}) + } + } + + complete_apps = ['survey'] \ No newline at end of file diff --git a/lms/djangoapps/survey/models.py b/lms/djangoapps/survey/models.py index 40e176bee4..88b9696364 100644 --- a/lms/djangoapps/survey/models.py +++ b/lms/djangoapps/survey/models.py @@ -13,6 +13,8 @@ from model_utils.models import TimeStampedModel from survey.exceptions import SurveyFormNameAlreadyExists, SurveyFormNotFound +from xmodule_django.models import CourseKeyField + log = logging.getLogger("edx.survey") @@ -104,7 +106,7 @@ class SurveyForm(TimeStampedModel): """ return SurveyAnswer.do_survey_answers_exist(self, user) - def save_user_answers(self, user, answers): + def save_user_answers(self, user, answers, course_key): """ Store answers to the form for a given user. Answers is a dict of simple name/value pairs @@ -112,7 +114,16 @@ class SurveyForm(TimeStampedModel): IMPORTANT: There is no validaton of form answers at this point. All data supplied to this method is presumed to be previously validated """ - SurveyAnswer.save_answers(self, user, answers) + + # first remove any answer the user might have done before + self.clear_user_answers(user) + SurveyAnswer.save_answers(self, user, answers, course_key) + + def clear_user_answers(self, user): + """ + Removes all answers that a user has submitted + """ + SurveyAnswer.objects.filter(form=self, user=user).delete() def get_field_names(self): """ @@ -135,7 +146,10 @@ class SurveyForm(TimeStampedModel): # NOTE: This wrapping doesn't change the ability to query it tree = etree.fromstring(u'
{}
'.format(html)) - input_fields = tree.findall('.//input') + tree.findall('.//select') + input_fields = ( + tree.findall('.//input') + tree.findall('.//select') + + tree.findall('.//textarea') + ) for input_field in input_fields: if 'name' in input_field.keys() and input_field.attrib['name'] not in names: @@ -153,6 +167,10 @@ class SurveyAnswer(TimeStampedModel): field_name = models.CharField(max_length=255, db_index=True) field_value = models.CharField(max_length=1024) + # adding the course_id where the end-user answered the survey question + # since it didn't exist in the beginning, it is nullable + course_key = CourseKeyField(max_length=255, db_index=True, null=True) + @classmethod def do_survey_answers_exist(cls, form, user): """ @@ -205,7 +223,7 @@ class SurveyAnswer(TimeStampedModel): return results @classmethod - def save_answers(cls, form, user, answers): + def save_answers(cls, form, user, answers, course_key): """ Store answers to the form for a given user. Answers is a dict of simple name/value pairs @@ -219,6 +237,20 @@ class SurveyAnswer(TimeStampedModel): # See if there is an answer stored for this user, form, field_name pair or not # this will allow for update cases. This does include an additional lookup, # but write operations will be relatively infrequent - answer, __ = SurveyAnswer.objects.get_or_create(user=user, form=form, field_name=name) - answer.field_value = value - answer.save() + value = answers[name] + defaults = {"field_value": value} + if course_key: + defaults['course_key'] = course_key + + answer, created = SurveyAnswer.objects.get_or_create( + user=user, + form=form, + field_name=name, + defaults=defaults + ) + + if not created: + # Allow for update cases. + answer.field_value = value + answer.course_key = course_key + answer.save() diff --git a/lms/djangoapps/survey/tests/test_models.py b/lms/djangoapps/survey/tests/test_models.py index 370fe53184..8edbea5d16 100644 --- a/lms/djangoapps/survey/tests/test_models.py +++ b/lms/djangoapps/survey/tests/test_models.py @@ -2,6 +2,7 @@ Python tests for the Survey models """ +import ddt from collections import OrderedDict from django.test import TestCase @@ -10,9 +11,10 @@ from django.contrib.auth.models import User from survey.exceptions import SurveyFormNotFound, SurveyFormNameAlreadyExists from django.core.exceptions import ValidationError -from survey.models import SurveyForm +from survey.models import SurveyForm, SurveyAnswer +@ddt.ddt class SurveyModelsTests(TestCase): """ All tests for the Survey models.py file @@ -32,12 +34,22 @@ class SurveyModelsTests(TestCase): self.test_survey_name = 'TestForm' self.test_form = '
    • ' self.test_form_update = '' + self.course_id = 'foo/bar/baz' self.student_answers = OrderedDict({ 'field1': 'value1', 'field2': 'value2', }) + self.student_answers_update = OrderedDict({ + 'field1': 'value1-updated', + 'field2': 'value2-updated', + }) + + self.student_answers_update2 = OrderedDict({ + 'field1': 'value1-updated2', + }) + self.student2_answers = OrderedDict({ 'field1': 'value3' }) @@ -142,7 +154,8 @@ class SurveyModelsTests(TestCase): self.assertFalse(survey.has_user_answered_survey(self.student)) self.assertEquals(len(survey.get_answers()), 0) - def test_single_user_answers(self): + @ddt.data(None, 'foo/bar/baz') + def test_single_user_answers(self, course_id): """ Create a new survey and add answers to it """ @@ -150,7 +163,7 @@ class SurveyModelsTests(TestCase): survey = self._create_test_survey() self.assertIsNotNone(survey) - survey.save_user_answers(self.student, self.student_answers) + survey.save_user_answers(self.student, self.student_answers, course_id) self.assertTrue(survey.has_user_answered_survey(self.student)) @@ -164,6 +177,19 @@ class SurveyModelsTests(TestCase): self.assertTrue(self.student.id in answers) self.assertEquals(all_answers[self.student.id], self.student_answers) + # check that the course_id was set + + answer_objs = SurveyAnswer.objects.filter( + user=self.student, + form=survey + ) + + for answer_obj in answer_objs: + if course_id: + self.assertEquals(unicode(answer_obj.course_key), course_id) + else: + self.assertIsNone(answer_obj.course_key) + def test_multiple_user_answers(self): """ Create a new survey and add answers to it @@ -172,8 +198,8 @@ class SurveyModelsTests(TestCase): survey = self._create_test_survey() self.assertIsNotNone(survey) - survey.save_user_answers(self.student, self.student_answers) - survey.save_user_answers(self.student2, self.student2_answers) + survey.save_user_answers(self.student, self.student_answers, self.course_id) + survey.save_user_answers(self.student2, self.student2_answers, self.course_id) self.assertTrue(survey.has_user_answered_survey(self.student)) @@ -187,12 +213,43 @@ class SurveyModelsTests(TestCase): answers = survey.get_answers(self.student) self.assertEquals(len(answers.keys()), 1) self.assertTrue(self.student.id in answers) - self.assertEquals(all_answers[self.student.id], self.student_answers) + self.assertEquals(answers[self.student.id], self.student_answers) answers = survey.get_answers(self.student2) self.assertEquals(len(answers.keys()), 1) self.assertTrue(self.student2.id in answers) - self.assertEquals(all_answers[self.student2.id], self.student2_answers) + self.assertEquals(answers[self.student2.id], self.student2_answers) + + def test_update_answers(self): + """ + Make sure the update case works + """ + + survey = self._create_test_survey() + self.assertIsNotNone(survey) + + survey.save_user_answers(self.student, self.student_answers, self.course_id) + + answers = survey.get_answers(self.student) + self.assertEquals(len(answers.keys()), 1) + self.assertTrue(self.student.id in answers) + self.assertEquals(answers[self.student.id], self.student_answers) + + # update + survey.save_user_answers(self.student, self.student_answers_update, self.course_id) + + answers = survey.get_answers(self.student) + self.assertEquals(len(answers.keys()), 1) + self.assertTrue(self.student.id in answers) + self.assertEquals(answers[self.student.id], self.student_answers_update) + + # update with just a subset of the origin dataset + survey.save_user_answers(self.student, self.student_answers_update2, self.course_id) + + answers = survey.get_answers(self.student) + self.assertEquals(len(answers.keys()), 1) + self.assertTrue(self.student.id in answers) + self.assertEquals(answers[self.student.id], self.student_answers_update2) def test_limit_num_users(self): """ @@ -201,8 +258,8 @@ class SurveyModelsTests(TestCase): """ survey = self._create_test_survey() - survey.save_user_answers(self.student, self.student_answers) - survey.save_user_answers(self.student2, self.student2_answers) + survey.save_user_answers(self.student, self.student_answers, self.course_id) + survey.save_user_answers(self.student2, self.student2_answers, self.course_id) # even though we have 2 users submitted answers # limit the result set to just 1 @@ -217,8 +274,8 @@ class SurveyModelsTests(TestCase): survey = self._create_test_survey() self.assertIsNotNone(survey) - survey.save_user_answers(self.student, self.student_answers) - survey.save_user_answers(self.student2, self.student2_answers) + survey.save_user_answers(self.student, self.student_answers, self.course_id) + survey.save_user_answers(self.student2, self.student2_answers, self.course_id) names = survey.get_field_names() diff --git a/lms/djangoapps/survey/tests/test_utils.py b/lms/djangoapps/survey/tests/test_utils.py index 320a4ad93c..00c1d3ea41 100644 --- a/lms/djangoapps/survey/tests/test_utils.py +++ b/lms/djangoapps/survey/tests/test_utils.py @@ -108,7 +108,7 @@ class SurveyModelsTests(ModuleStoreTestCase): """ Assert that a new course which has a required survey and user has answers for it """ - self.survey.save_user_answers(self.student, self.student_answers) + self.survey.save_user_answers(self.student, self.student_answers, None) self.assertFalse(must_answer_survey(self.course, self.student)) def test_staff_must_answer_survey(self): diff --git a/lms/djangoapps/survey/tests/test_views.py b/lms/djangoapps/survey/tests/test_views.py index 2d1d718e87..66f4c3f0c7 100644 --- a/lms/djangoapps/survey/tests/test_views.py +++ b/lms/djangoapps/survey/tests/test_views.py @@ -9,7 +9,7 @@ from django.test.client import Client from django.contrib.auth.models import User from django.core.urlresolvers import reverse -from survey.models import SurveyForm +from survey.models import SurveyForm, SurveyAnswer from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -32,15 +32,20 @@ class SurveyViewsTests(ModuleStoreTestCase): self.student = User.objects.create_user('student', 'student@test.com', self.password) self.test_survey_name = 'TestSurvey' - self.test_form = '' + self.test_form = ''' + +