From 73af5d017ec296f58f6a67c28b1665a0d4fc3bfe Mon Sep 17 00:00:00 2001 From: Julia Hansbrough Date: Fri, 8 Nov 2013 19:01:51 +0000 Subject: [PATCH 1/6] Changed create_enrollment to create_or_update_enrollment --- common/djangoapps/student/models.py | 4 ++-- common/djangoapps/student/tests/tests.py | 2 +- lms/djangoapps/shoppingcart/models.py | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 6387f282e1..b648adf51d 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -713,7 +713,7 @@ class CourseEnrollment(models.Model): ).format(self.user, self.course_id, self.created, self.is_active) @classmethod - def create_enrollment(cls, user, course_id, mode="honor", is_active=False): + def create_or_update_enrollment(cls, user, course_id, mode="honor", is_active=False): """ Create an enrollment for a user in a class. By default *this enrollment is not active*. This is useful for when an enrollment needs to go @@ -818,7 +818,7 @@ class CourseEnrollment(models.Model): It is expected that this method is called from a method which has already verified the user authentication and access. """ - return cls.create_enrollment(user, course_id, mode, is_active=True) + return cls.create_or_update_enrollment(user, course_id, mode, is_active=True) @classmethod def enroll_by_email(cls, email, course_id, mode="honor", ignore_errors=True): diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index 634996fd55..2239259a1d 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/common/djangoapps/student/tests/tests.py @@ -443,7 +443,7 @@ class EnrollInCourseTest(TestCase): # Creating an enrollment doesn't actually enroll a student # (calling CourseEnrollment.enroll() would have) - enrollment = CourseEnrollment.create_enrollment(user, course_id) + enrollment = CourseEnrollment.create_or_update_enrollment(user, course_id) self.assertFalse(CourseEnrollment.is_enrolled(user, course_id)) self.assert_no_events_were_emitted() diff --git a/lms/djangoapps/shoppingcart/models.py b/lms/djangoapps/shoppingcart/models.py index 03be80861a..8a91a77286 100644 --- a/lms/djangoapps/shoppingcart/models.py +++ b/lms/djangoapps/shoppingcart/models.py @@ -467,7 +467,7 @@ class CertificateItem(OrderItem): try: course_enrollment = CourseEnrollment.objects.get(user=order.user, course_id=course_id) except ObjectDoesNotExist: - course_enrollment = CourseEnrollment.create_enrollment(order.user, course_id, mode=mode) + course_enrollment = CourseEnrollment.create_or_update_enrollment(order.user, course_id, mode=mode) # do some validation on the enrollment mode valid_modes = CourseMode.modes_for_course_dict(course_id) From 94b8b0acc92b17520d828e1d9b322665c6bb5fb9 Mon Sep 17 00:00:00 2001 From: Julia Hansbrough Date: Fri, 8 Nov 2013 20:14:17 +0000 Subject: [PATCH 2/6] all CourseEnrollment modifications route through create_or_update --- common/djangoapps/student/models.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index b648adf51d..3b830d356a 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -872,12 +872,11 @@ class CourseEnrollment(models.Model): """ try: record = CourseEnrollment.objects.get(user=user, course_id=course_id) - if record.is_active: - record.is_active = False - record.save() + cls.create_or_update_enrollment(user, course_id, record.mode, is_active=False) + unenroll_done.send(sender=cls, course_enrollment=record) - record.emit_event(EVENT_NAME_ENROLLMENT_DEACTIVATED) - unenroll_done.send(sender=cls, course_enrollment=record) + # TODO: Do we still need to emit this event since unenroll now calls create_or_update_enrollment? + #record.emit_event(EVENT_NAME_ENROLLMENT_DEACTIVATED) except cls.DoesNotExist: err_msg = u"Tried to unenroll student {} from {} but they were not enrolled" @@ -968,8 +967,7 @@ class CourseEnrollment(models.Model): """Makes this `CourseEnrollment` record active. Saves immediately.""" if not self.is_active: self.is_active = True - self.save() - self.emit_event(EVENT_NAME_ENROLLMENT_ACTIVATED) + CourseEnrollment.create_or_update_enrollment(self.user, self.course_id, self.mode, True) def deactivate(self): """Makes this `CourseEnrollment` record inactive. Saves immediately. An @@ -977,8 +975,11 @@ class CourseEnrollment(models.Model): """ if self.is_active: self.is_active = False - self.save() - self.emit_event(EVENT_NAME_ENROLLMENT_DEACTIVATED) + CourseEnrollment.create_or_update_enrollment(self.user, self.course_id, self.mode, False) + + def change_mode(self, mode): + self.mode = mode + CourseEnrollment.create_or_update_enrollment(self.user, self.course_id, mode, self.is_active) def refundable(self): """ @@ -992,7 +993,6 @@ class CourseEnrollment(models.Model): return True - class CourseEnrollmentAllowed(models.Model): """ Table of users (specified by email address strings) who are allowed to enroll in a specified course. From 8042332cda971a73344701054dd56e9413a62967 Mon Sep 17 00:00:00 2001 From: Julia Hansbrough Date: Fri, 8 Nov 2013 20:21:45 +0000 Subject: [PATCH 3/6] purchased_callback now uses the change_mode method --- common/djangoapps/student/models.py | 4 +--- lms/djangoapps/shoppingcart/models.py | 3 +-- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 3b830d356a..f8b9e70ab0 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -875,9 +875,6 @@ class CourseEnrollment(models.Model): cls.create_or_update_enrollment(user, course_id, record.mode, is_active=False) unenroll_done.send(sender=cls, course_enrollment=record) - # TODO: Do we still need to emit this event since unenroll now calls create_or_update_enrollment? - #record.emit_event(EVENT_NAME_ENROLLMENT_DEACTIVATED) - except cls.DoesNotExist: err_msg = u"Tried to unenroll student {} from {} but they were not enrolled" log.error(err_msg.format(user, course_id)) @@ -978,6 +975,7 @@ class CourseEnrollment(models.Model): CourseEnrollment.create_or_update_enrollment(self.user, self.course_id, self.mode, False) def change_mode(self, mode): + """Changes this `CourseEnrollment` record's mode to `mode`. Saves immediately.""" self.mode = mode CourseEnrollment.create_or_update_enrollment(self.user, self.course_id, mode, self.is_active) diff --git a/lms/djangoapps/shoppingcart/models.py b/lms/djangoapps/shoppingcart/models.py index 8a91a77286..160769946c 100644 --- a/lms/djangoapps/shoppingcart/models.py +++ b/lms/djangoapps/shoppingcart/models.py @@ -506,8 +506,7 @@ class CertificateItem(OrderItem): "Could not submit verification attempt for enrollment {}".format(self.course_enrollment) ) - self.course_enrollment.mode = self.mode - self.course_enrollment.save() + self.course_enrollment.change_mode(self.mode) self.course_enrollment.activate() @property From e73489ec31fff91acd3bc0aef0fb44d88f4a32a2 Mon Sep 17 00:00:00 2001 From: Julia Hansbrough Date: Thu, 14 Nov 2013 21:42:46 +0000 Subject: [PATCH 4/6] Response to CR --- common/djangoapps/student/models.py | 75 ++++++++++++------------ common/djangoapps/student/tests/tests.py | 2 +- lms/djangoapps/shoppingcart/models.py | 7 ++- 3 files changed, 41 insertions(+), 43 deletions(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index f8b9e70ab0..ebc9f4028c 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -713,7 +713,7 @@ class CourseEnrollment(models.Model): ).format(self.user, self.course_id, self.created, self.is_active) @classmethod - def create_or_update_enrollment(cls, user, course_id, mode="honor", is_active=False): + def create_enrollment(cls, user, course_id): """ Create an enrollment for a user in a class. By default *this enrollment is not active*. This is useful for when an enrollment needs to go @@ -728,16 +728,6 @@ class CourseEnrollment(models.Model): `course_id` is our usual course_id string (e.g. "edX/Test101/2013_Fall) - `mode` is a string specifying what kind of enrollment this is. The - default is "honor", meaning honor certificate. Future options - may include "audit", "verified_id", etc. Please don't use it - until we have these mapped out. - - `is_active` is a boolean. If the CourseEnrollment object has - `is_active=False`, then calling - `CourseEnrollment.is_enrolled()` for that user/course_id - will return False. - It is expected that this method is called from a method which has already verified the user authentication and access. """ @@ -753,30 +743,41 @@ class CourseEnrollment(models.Model): course_id=course_id, ) + # If we *did* just create a new enrollment, set some defaults + if created: + enrollment.mode = "honor" + enrollment.is_active = False + enrollment.save() + + return enrollment + + def update_enrollment(self, mode=None, is_active=None): + """ + Updates an enrollment for a user in a class. This includes options + like changing the mode, toggling is_active True/False, etc. + + Also emits relevant events for analytics purposes. + + This saves immediately. + """ activation_changed = False - if enrollment.is_active != is_active: - enrollment.is_active = is_active + if self.is_active != is_active and is_active is not None: + self.is_active = is_active activation_changed = True mode_changed = False - if enrollment.mode != mode: - enrollment.mode = mode + if self.mode != mode and mode is not None: + self.mode = mode mode_changed = True if activation_changed or mode_changed: - enrollment.save() - - if created: - if is_active: - enrollment.emit_event(EVENT_NAME_ENROLLMENT_ACTIVATED) - else: - if activation_changed: - if is_active: - enrollment.emit_event(EVENT_NAME_ENROLLMENT_ACTIVATED) - else: - enrollment.emit_event(EVENT_NAME_ENROLLMENT_DEACTIVATED) - - return enrollment + self.save() + if activation_changed: + if self.is_active: + self.emit_event(EVENT_NAME_ENROLLMENT_ACTIVATED) + else: + unenroll_done.send(sender=None, course_enrollment=self) + self.emit_event(EVENT_NAME_ENROLLMENT_DEACTIVATED) def emit_event(self, event_name): """ @@ -818,7 +819,9 @@ class CourseEnrollment(models.Model): It is expected that this method is called from a method which has already verified the user authentication and access. """ - return cls.create_or_update_enrollment(user, course_id, mode, is_active=True) + enrollment = cls.create_enrollment(user, course_id) + enrollment.update_enrollment(is_active=True) + return enrollment @classmethod def enroll_by_email(cls, email, course_id, mode="honor", ignore_errors=True): @@ -872,8 +875,7 @@ class CourseEnrollment(models.Model): """ try: record = CourseEnrollment.objects.get(user=user, course_id=course_id) - cls.create_or_update_enrollment(user, course_id, record.mode, is_active=False) - unenroll_done.send(sender=cls, course_enrollment=record) + record.update_enrollment(is_active=False) except cls.DoesNotExist: err_msg = u"Tried to unenroll student {} from {} but they were not enrolled" @@ -962,22 +964,17 @@ class CourseEnrollment(models.Model): def activate(self): """Makes this `CourseEnrollment` record active. Saves immediately.""" - if not self.is_active: - self.is_active = True - CourseEnrollment.create_or_update_enrollment(self.user, self.course_id, self.mode, True) + self.update_enrollment(is_active=True) def deactivate(self): """Makes this `CourseEnrollment` record inactive. Saves immediately. An inactive record means that the student is not enrolled in this course. """ - if self.is_active: - self.is_active = False - CourseEnrollment.create_or_update_enrollment(self.user, self.course_id, self.mode, False) + self.update_enrollment(is_active=False) def change_mode(self, mode): """Changes this `CourseEnrollment` record's mode to `mode`. Saves immediately.""" - self.mode = mode - CourseEnrollment.create_or_update_enrollment(self.user, self.course_id, mode, self.is_active) + self.update_enrollment(mode=mode) def refundable(self): """ diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index 2239259a1d..634996fd55 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/common/djangoapps/student/tests/tests.py @@ -443,7 +443,7 @@ class EnrollInCourseTest(TestCase): # Creating an enrollment doesn't actually enroll a student # (calling CourseEnrollment.enroll() would have) - enrollment = CourseEnrollment.create_or_update_enrollment(user, course_id) + enrollment = CourseEnrollment.create_enrollment(user, course_id) self.assertFalse(CourseEnrollment.is_enrolled(user, course_id)) self.assert_no_events_were_emitted() diff --git a/lms/djangoapps/shoppingcart/models.py b/lms/djangoapps/shoppingcart/models.py index 160769946c..ceac6c87e0 100644 --- a/lms/djangoapps/shoppingcart/models.py +++ b/lms/djangoapps/shoppingcart/models.py @@ -400,8 +400,8 @@ class CertificateItem(OrderItem): course_enrollment = models.ForeignKey(CourseEnrollment) mode = models.SlugField() - @receiver(unenroll_done, sender=CourseEnrollment) - def refund_cert_callback(sender, course_enrollment=None, **kwargs): + @receiver(unenroll_done) + def refund_cert_callback(sender, course_enrollment=course_enrollment, **kwargs): """ When a CourseEnrollment object calls its unenroll method, this function checks to see if that unenrollment occurred in a verified certificate that was within the refund deadline. If so, it actually performs the @@ -467,7 +467,8 @@ class CertificateItem(OrderItem): try: course_enrollment = CourseEnrollment.objects.get(user=order.user, course_id=course_id) except ObjectDoesNotExist: - course_enrollment = CourseEnrollment.create_or_update_enrollment(order.user, course_id, mode=mode) + course_enrollment = CourseEnrollment.create_enrollment(order.user, course_id) + course_enrollment.update_enrollment(mode=mode) # do some validation on the enrollment mode valid_modes = CourseMode.modes_for_course_dict(course_id) From f950ea106d5d080dc9a5fa1b8177446a96b449bb Mon Sep 17 00:00:00 2001 From: Julia Hansbrough Date: Fri, 15 Nov 2013 15:38:21 +0000 Subject: [PATCH 5/6] response to CR --- common/djangoapps/student/models.py | 8 ++++++-- common/djangoapps/student/tests/tests.py | 2 +- lms/djangoapps/shoppingcart/models.py | 7 ++----- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index ebc9f4028c..f97085c97f 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -713,7 +713,7 @@ class CourseEnrollment(models.Model): ).format(self.user, self.course_id, self.created, self.is_active) @classmethod - def create_enrollment(cls, user, course_id): + def get_or_create_enrollment(cls, user, course_id): """ Create an enrollment for a user in a class. By default *this enrollment is not active*. This is useful for when an enrollment needs to go @@ -761,11 +761,15 @@ class CourseEnrollment(models.Model): This saves immediately. """ activation_changed = False + # if is_active is None, then the call to update_enrollment didn't specify + # any value, so just leave is_active as it is if self.is_active != is_active and is_active is not None: self.is_active = is_active activation_changed = True mode_changed = False + # if mode is None, the call to update_enrollment didn't specify a new + # mode, so leave as-is if self.mode != mode and mode is not None: self.mode = mode mode_changed = True @@ -819,7 +823,7 @@ class CourseEnrollment(models.Model): It is expected that this method is called from a method which has already verified the user authentication and access. """ - enrollment = cls.create_enrollment(user, course_id) + enrollment = cls.get_or_create_enrollment(user, course_id) enrollment.update_enrollment(is_active=True) return enrollment diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index 634996fd55..7e19ee359e 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/common/djangoapps/student/tests/tests.py @@ -443,7 +443,7 @@ class EnrollInCourseTest(TestCase): # Creating an enrollment doesn't actually enroll a student # (calling CourseEnrollment.enroll() would have) - enrollment = CourseEnrollment.create_enrollment(user, course_id) + enrollment = CourseEnrollment.get_or_create_enrollment(user, course_id) self.assertFalse(CourseEnrollment.is_enrolled(user, course_id)) self.assert_no_events_were_emitted() diff --git a/lms/djangoapps/shoppingcart/models.py b/lms/djangoapps/shoppingcart/models.py index ceac6c87e0..e072a79764 100644 --- a/lms/djangoapps/shoppingcart/models.py +++ b/lms/djangoapps/shoppingcart/models.py @@ -464,11 +464,8 @@ class CertificateItem(OrderItem): """ super(CertificateItem, cls).add_to_order(order, course_id, cost, currency=currency) - try: - course_enrollment = CourseEnrollment.objects.get(user=order.user, course_id=course_id) - except ObjectDoesNotExist: - course_enrollment = CourseEnrollment.create_enrollment(order.user, course_id) - course_enrollment.update_enrollment(mode=mode) + course_enrollment = CourseEnrollment.get_or_create_enrollment(order.user, course_id) + course_enrollment.update_enrollment(mode=mode) # do some validation on the enrollment mode valid_modes = CourseMode.modes_for_course_dict(course_id) From b075143d62753bc2d6d3e5aecd34aed0080fbe6b Mon Sep 17 00:00:00 2001 From: Julia Hansbrough Date: Fri, 15 Nov 2013 16:27:48 +0000 Subject: [PATCH 6/6] Small fix --- lms/djangoapps/shoppingcart/models.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lms/djangoapps/shoppingcart/models.py b/lms/djangoapps/shoppingcart/models.py index e072a79764..8d3ecfa32f 100644 --- a/lms/djangoapps/shoppingcart/models.py +++ b/lms/djangoapps/shoppingcart/models.py @@ -401,7 +401,7 @@ class CertificateItem(OrderItem): mode = models.SlugField() @receiver(unenroll_done) - def refund_cert_callback(sender, course_enrollment=course_enrollment, **kwargs): + def refund_cert_callback(sender, course_enrollment=None, **kwargs): """ When a CourseEnrollment object calls its unenroll method, this function checks to see if that unenrollment occurred in a verified certificate that was within the refund deadline. If so, it actually performs the @@ -465,7 +465,6 @@ class CertificateItem(OrderItem): """ super(CertificateItem, cls).add_to_order(order, course_id, cost, currency=currency) course_enrollment = CourseEnrollment.get_or_create_enrollment(order.user, course_id) - course_enrollment.update_enrollment(mode=mode) # do some validation on the enrollment mode valid_modes = CourseMode.modes_for_course_dict(course_id)