From b5beb13964210089d5411bf129541c90acace9e5 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 13 Aug 2012 13:06:35 -0400 Subject: [PATCH 1/4] Fix side-effect related problems with User replication. 1. Multiple save()s on the same model are now handled properly. We had to unmark model objects after the appropriate signals had fired. 2. There was a side-effect where we were saving the portal User object to the course_db with the using kw param, but models remember where they were last saved to, so a later save on that model object would go to the wrong database. --- common/djangoapps/student/models.py | 48 ++++++++++++++--------------- common/djangoapps/student/tests.py | 29 +++++++++++------ 2 files changed, 44 insertions(+), 33 deletions(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 0fbe70c0b3..a3f6926a51 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -255,10 +255,13 @@ def add_user_to_default_group(user, group): utg.save() ########################## REPLICATION SIGNALS ################################# -@receiver(post_save, sender=User) +#@receiver(post_save, sender=User) def replicate_user_save(sender, **kwargs): - user_obj = kwargs['instance'] - return replicate_model(User.save, user_obj, user_obj.id) + user_obj = kwargs['instance'] + if not should_replicate(user_obj): + return + for course_db_name in db_names_to_replicate_to(user_obj.id): + replicate_user(user_obj, course_db_name) @receiver(post_save, sender=CourseEnrollment) def replicate_enrollment_save(sender, **kwargs): @@ -287,8 +290,8 @@ def replicate_enrollment_save(sender, **kwargs): @receiver(post_delete, sender=CourseEnrollment) def replicate_enrollment_delete(sender, **kwargs): - enrollment_obj = kwargs['instance'] - return replicate_model(CourseEnrollment.delete, enrollment_obj, enrollment_obj.user_id) + enrollment_obj = kwargs['instance'] + return replicate_model(CourseEnrollment.delete, enrollment_obj, enrollment_obj.user_id) @receiver(post_save, sender=UserProfile) def replicate_userprofile_save(sender, **kwargs): @@ -311,23 +314,20 @@ def replicate_user(portal_user, course_db_name): overridden. """ try: - # If the user exists in the Course DB, update the appropriate fields and - # save it back out to the Course DB. course_user = User.objects.using(course_db_name).get(id=portal_user.id) - for field in USER_FIELDS_TO_COPY: - setattr(course_user, field, getattr(portal_user, field)) - - mark_handled(course_user) log.debug("User {0} found in Course DB, replicating fields to {1}" .format(course_user, course_db_name)) - course_user.save(using=course_db_name) # Just being explicit. - except User.DoesNotExist: - # Otherwise, just make a straight copy to the Course DB. - mark_handled(portal_user) log.debug("User {0} not found in Course DB, creating copy in {1}" .format(portal_user, course_db_name)) - portal_user.save(using=course_db_name) + course_user = User() + + for field in USER_FIELDS_TO_COPY: + setattr(course_user, field, getattr(portal_user, field)) + + mark_handled(course_user) + course_user.save(using=course_db_name) # Just being explicit. + unmark(course_user) def replicate_model(model_method, instance, user_id): """ @@ -337,13 +337,14 @@ def replicate_model(model_method, instance, user_id): if not should_replicate(instance): return - mark_handled(instance) course_db_names = db_names_to_replicate_to(user_id) log.debug("Replicating {0} for user {1} to DBs: {2}" .format(model_method, user_id, course_db_names)) + mark_handled(instance) for db_name in course_db_names: model_method(instance, using=db_name) + unmark(instance) ######### Replication Helpers ######### @@ -371,7 +372,7 @@ def db_names_to_replicate_to(user_id): def marked_handled(instance): """Have we marked this instance as being handled to avoid infinite loops caused by saving models in post_save hooks for the same models?""" - return hasattr(instance, '_do_not_copy_to_course_db') + return hasattr(instance, '_do_not_copy_to_course_db') and instance._do_not_copy_to_course_db def mark_handled(instance): """You have to mark your instance with this function or else we'll go into @@ -384,6 +385,11 @@ def mark_handled(instance): """ instance._do_not_copy_to_course_db = True +def unmark(instance): + """If we don't unmark a model after we do replication, then consecutive + save() calls won't be properly replicated.""" + instance._do_not_copy_to_course_db = False + def should_replicate(instance): """Should this instance be replicated? We need to be a Portal server and the instance has to not have been marked_handled.""" @@ -398,9 +404,3 @@ def should_replicate(instance): return False return True - - - - - - diff --git a/common/djangoapps/student/tests.py b/common/djangoapps/student/tests.py index ad7ddb70d1..b71fce5158 100644 --- a/common/djangoapps/student/tests.py +++ b/common/djangoapps/student/tests.py @@ -4,6 +4,7 @@ when you run "manage.py test". Replace this with more appropriate tests for your application. """ +import logging from datetime import datetime from django.test import TestCase @@ -13,6 +14,8 @@ from .models import User, UserProfile, CourseEnrollment, replicate_user, USER_FI COURSE_1 = 'edX/toy/2012_Fall' COURSE_2 = 'edx/full/6.002_Spring_2012' +log = logging.getLogger(__name__) + class ReplicationTest(TestCase): multi_db = True @@ -47,23 +50,18 @@ class ReplicationTest(TestCase): field, portal_user, course_user )) - if hasattr(portal_user, 'seen_response_count'): - # Since it's the first copy over of User data, we should have all of it - self.assertEqual(portal_user.seen_response_count, - course_user.seen_response_count) - - # But if we replicate again, the user already exists in the Course DB, - # so it shouldn't update the seen_response_count (which is Askbot - # controlled). # This hasattr lameness is here because we don't want this test to be # triggered when we're being run by CMS tests (Askbot doesn't exist # there, so the test will fail). + # + # seen_response_count isn't a field we care about, so it shouldn't have + # been copied over. if hasattr(portal_user, 'seen_response_count'): portal_user.seen_response_count = 20 replicate_user(portal_user, COURSE_1) course_user = User.objects.using(COURSE_1).get(id=portal_user.id) self.assertEqual(portal_user.seen_response_count, 20) - self.assertEqual(course_user.seen_response_count, 10) + self.assertEqual(course_user.seen_response_count, 0) # Another replication should work for an email change however, since # it's a field we care about. @@ -123,6 +121,19 @@ class ReplicationTest(TestCase): UserProfile.objects.using(COURSE_2).get, id=portal_user_profile.id) + log.debug("Make sure our seen_response_count is not replicated.") + if hasattr(portal_user, 'seen_response_count'): + portal_user.seen_response_count = 200 + course_user = User.objects.using(COURSE_1).get(id=portal_user.id) + self.assertEqual(portal_user.seen_response_count, 200) + self.assertEqual(course_user.seen_response_count, 0) + portal_user.save() + + course_user = User.objects.using(COURSE_1).get(id=portal_user.id) + self.assertEqual(portal_user.seen_response_count, 200) + self.assertEqual(course_user.seen_response_count, 0) + + def test_enrollment_for_user_info_after_enrollment(self): """Test the effect of modifying User data after you've enrolled.""" From 301695a5c4e6913ff2e15529be12941d8e3d34a1 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 13 Aug 2012 13:11:27 -0400 Subject: [PATCH 2/4] Remove outdated comment --- common/djangoapps/student/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index a3f6926a51..d04a56362e 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -326,7 +326,7 @@ def replicate_user(portal_user, course_db_name): setattr(course_user, field, getattr(portal_user, field)) mark_handled(course_user) - course_user.save(using=course_db_name) # Just being explicit. + course_user.save(using=course_db_name) unmark(course_user) def replicate_model(model_method, instance, user_id): From a25f289ca7d14bbdb3bf3c6a62fd02b694c40d8c Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 13 Aug 2012 13:16:33 -0400 Subject: [PATCH 3/4] re-enable User save signal handler --- common/djangoapps/student/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index d04a56362e..6d1cbb5afb 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -255,7 +255,7 @@ def add_user_to_default_group(user, group): utg.save() ########################## REPLICATION SIGNALS ################################# -#@receiver(post_save, sender=User) +@receiver(post_save, sender=User) def replicate_user_save(sender, **kwargs): user_obj = kwargs['instance'] if not should_replicate(user_obj): From c4d89cd535cc9afa04a719e23d83bc35527033bc Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 13 Aug 2012 13:34:24 -0400 Subject: [PATCH 4/4] One more test to make sure users are really being copied --- common/djangoapps/student/tests.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/common/djangoapps/student/tests.py b/common/djangoapps/student/tests.py index b71fce5158..b33678fbac 100644 --- a/common/djangoapps/student/tests.py +++ b/common/djangoapps/student/tests.py @@ -133,6 +133,12 @@ class ReplicationTest(TestCase): self.assertEqual(portal_user.seen_response_count, 200) self.assertEqual(course_user.seen_response_count, 0) + portal_user.email = 'jim@edx.org' + portal_user.save() + course_user = User.objects.using(COURSE_1).get(id=portal_user.id) + self.assertEqual(portal_user.email, 'jim@edx.org') + self.assertEqual(course_user.email, 'jim@edx.org') + def test_enrollment_for_user_info_after_enrollment(self):