From 551f690adecf7869c0cd9eb59e5cb631201f06c2 Mon Sep 17 00:00:00 2001 From: Waheed Ahmed Date: Thu, 26 Sep 2019 15:20:00 +0500 Subject: [PATCH 1/2] Use order date_placed enrollment attribute for refund. Use order date_placed enrollment attribute for refund if exist otherwise fetch it from ecommerce. PROD-454 --- common/djangoapps/student/models.py | 70 +++++++++++-------- .../djangoapps/student/tests/test_refunds.py | 28 +++++++- 2 files changed, 67 insertions(+), 31 deletions(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 9a7e521eb8..648bda9a80 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -1765,8 +1765,45 @@ class CourseEnrollment(models.Model): # NOTE: This is here to avoid circular references from openedx.core.djangoapps.commerce.utils import ecommerce_api_client, ECOMMERCE_DATE_FORMAT + date_placed = self.get_order_attribute_value('date_placed') + + if not date_placed: + order_number = self.get_order_attribute_value('order_number') + if not order_number: + return None + + try: + order = ecommerce_api_client(self.user).orders(order_number).get() + date_placed = order['date_placed'] + except HttpClientError: + log.warning( + u"Encountered HttpClientError while getting order details from ecommerce. " + u"Order={number} and user {user}".format(number=order_number, user=self.user.id)) + return None + + except HttpServerError: + log.warning( + u"Encountered HttpServerError while getting order details from ecommerce. " + u"Order={number} and user {user}".format(number=order_number, user=self.user.id)) + return None + + except SlumberBaseException: + log.warning( + u"Encountered an error while getting order details from ecommerce. " + u"Order={number} and user {user}".format(number=order_number, user=self.user.id)) + return None + + refund_window_start_date = max( + datetime.strptime(date_placed, ECOMMERCE_DATE_FORMAT), + self.course_overview.start.replace(tzinfo=None) + ) + + return refund_window_start_date.replace(tzinfo=UTC) + EnrollmentRefundConfiguration.current().refund_window + + def get_order_attribute_value(self, attr_name): + """ Get and return course enrollment order attribute's value.""" try: - attribute = self.attributes.get(namespace='order', name='order_number') + attribute = self.attributes.get(namespace='order', name=attr_name) except ObjectDoesNotExist: return None except MultipleObjectsReturned: @@ -1777,36 +1814,9 @@ class CourseEnrollment(models.Model): self.user.id, enrollment_id ) - attribute = self.attributes.filter(namespace='order', name='order_number').last() + attribute = self.attributes.filter(namespace='order', name=attr_name).last() - order_number = attribute.value - try: - order = ecommerce_api_client(self.user).orders(order_number).get() - - except HttpClientError: - log.warning( - u"Encountered HttpClientError while getting order details from ecommerce. " - u"Order={number} and user {user}".format(number=order_number, user=self.user.id)) - return None - - except HttpServerError: - log.warning( - u"Encountered HttpServerError while getting order details from ecommerce. " - u"Order={number} and user {user}".format(number=order_number, user=self.user.id)) - return None - - except SlumberBaseException: - log.warning( - u"Encountered an error while getting order details from ecommerce. " - u"Order={number} and user {user}".format(number=order_number, user=self.user.id)) - return None - - refund_window_start_date = max( - datetime.strptime(order['date_placed'], ECOMMERCE_DATE_FORMAT), - self.course_overview.start.replace(tzinfo=None) - ) - - return refund_window_start_date.replace(tzinfo=UTC) + EnrollmentRefundConfiguration.current().refund_window + return attribute.value @property def username(self): diff --git a/common/djangoapps/student/tests/test_refunds.py b/common/djangoapps/student/tests/test_refunds.py index 53a3d15e92..2214946d6f 100644 --- a/common/djangoapps/student/tests/test_refunds.py +++ b/common/djangoapps/student/tests/test_refunds.py @@ -22,10 +22,11 @@ from six.moves import range # These imports refer to lms djangoapps. # Their testcases are only run under lms. from course_modes.tests.factories import CourseModeFactory + from lms.djangoapps.certificates.models import CertificateStatuses, GeneratedCertificate from lms.djangoapps.certificates.tests.factories import GeneratedCertificateFactory from openedx.core.djangoapps.commerce.utils import ECOMMERCE_DATE_FORMAT -from student.models import CourseEnrollment +from student.models import CourseEnrollment, EnrollmentRefundConfiguration from student.tests.factories import UserFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -169,6 +170,31 @@ class RefundableTest(SharedModuleStoreTestCase): """ Assert that the None is returned when no order number attribute is found.""" self.assertIsNone(self.enrollment.refund_cutoff_date()) + @patch('openedx.core.djangoapps.commerce.utils.ecommerce_api_client') + def test_refund_cutoff_date_with_date_placed_attr(self, mock_ecommerce_api_client): + """ + Assert that the refund_cutoff_date returns order placement date if order:date_placed + attribute exist without calling ecommerce. + """ + now = datetime.now(pytz.UTC).replace(microsecond=0) + order_date = now + timedelta(days=2) + course_start = now + timedelta(days=1) + + self.enrollment.course_overview.start = course_start + self.enrollment.attributes.create( + enrollment=self.enrollment, + namespace='order', + name='date_placed', + value=order_date.strftime(ECOMMERCE_DATE_FORMAT) + ) + + refund_config = EnrollmentRefundConfiguration.current() + self.assertEqual( + self.enrollment.refund_cutoff_date(), + order_date + refund_config.refund_window + ) + mock_ecommerce_api_client.assert_not_called() + @httpretty.activate @override_settings(ECOMMERCE_API_URL=TEST_API_URL) def test_multiple_refunds_dashbaord_page_error(self): From 68b5f98485fb533f0928cb289cd67d7875f40988 Mon Sep 17 00:00:00 2001 From: Waheed Ahmed Date: Thu, 26 Sep 2019 16:10:55 +0500 Subject: [PATCH 2/2] Save order date attribute and some refactoring. Save order placement date attribute so that we don't need to call ecommerce again. Also did some refactoring. --- common/djangoapps/student/models.py | 17 ++++- .../djangoapps/student/tests/test_refunds.py | 17 ++++- openedx/core/djangoapps/enrollments/api.py | 64 ++++++++++--------- openedx/core/djangoapps/enrollments/data.py | 44 ++++++------- 4 files changed, 85 insertions(+), 57 deletions(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 648bda9a80..c9b0991c06 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -65,7 +65,11 @@ from lms.djangoapps.certificates.models import GeneratedCertificate from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification from openedx.core.djangoapps.content.course_overviews.models import CourseOverview import openedx.core.djangoapps.django_comment_common.comment_client as cc -from openedx.core.djangoapps.enrollments.api import _default_course_mode +from openedx.core.djangoapps.enrollments.api import ( + _default_course_mode, + get_enrollment_attributes, + set_enrollment_attributes +) from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.xmodule_django.models import NoneToEmptyManager from openedx.core.djangolib.model_mixins import DeletableByUserValue @@ -1775,6 +1779,17 @@ class CourseEnrollment(models.Model): try: order = ecommerce_api_client(self.user).orders(order_number).get() date_placed = order['date_placed'] + # also save the attribute so that we don't need to call ecommerce again. + username = self.user.username + enrollment_attributes = get_enrollment_attributes(username, six.text_type(self.course_id)) + enrollment_attributes.append( + { + "namespace": "order", + "name": "date_placed", + "value": date_placed, + } + ) + set_enrollment_attributes(username, six.text_type(self.course_id), enrollment_attributes) except HttpClientError: log.warning( u"Encountered HttpClientError while getting order details from ecommerce. " diff --git a/common/djangoapps/student/tests/test_refunds.py b/common/djangoapps/student/tests/test_refunds.py index 2214946d6f..67de106610 100644 --- a/common/djangoapps/student/tests/test_refunds.py +++ b/common/djangoapps/student/tests/test_refunds.py @@ -22,11 +22,10 @@ from six.moves import range # These imports refer to lms djangoapps. # Their testcases are only run under lms. from course_modes.tests.factories import CourseModeFactory - from lms.djangoapps.certificates.models import CertificateStatuses, GeneratedCertificate from lms.djangoapps.certificates.tests.factories import GeneratedCertificateFactory from openedx.core.djangoapps.commerce.utils import ECOMMERCE_DATE_FORMAT -from student.models import CourseEnrollment, EnrollmentRefundConfiguration +from student.models import CourseEnrollment, CourseEnrollmentAttribute, EnrollmentRefundConfiguration from student.tests.factories import UserFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -141,7 +140,8 @@ class RefundableTest(SharedModuleStoreTestCase): course_start = now + course_start_delta expected_date = now + expected_date_delta refund_period = timedelta(days=days) - expected_content = '{{"date_placed": "{date}"}}'.format(date=order_date.strftime(ECOMMERCE_DATE_FORMAT)) + date_placed = order_date.strftime(ECOMMERCE_DATE_FORMAT) + expected_content = '{{"date_placed": "{date}"}}'.format(date=date_placed) httpretty.register_uri( httpretty.GET, @@ -166,6 +166,17 @@ class RefundableTest(SharedModuleStoreTestCase): expected_date + refund_period ) + expected_date_placed_attr = { + "namespace": "order", + "name": "date_placed", + "value": date_placed, + } + + self.assertIn( + expected_date_placed_attr, + CourseEnrollmentAttribute.get_enrollment_attributes(self.enrollment) + ) + def test_refund_cutoff_date_no_attributes(self): """ Assert that the None is returned when no order number attribute is found.""" self.assertIsNone(self.enrollment.refund_cutoff_date()) diff --git a/openedx/core/djangoapps/enrollments/api.py b/openedx/core/djangoapps/enrollments/api.py index c3b68f2701..f23b697762 100644 --- a/openedx/core/djangoapps/enrollments/api.py +++ b/openedx/core/djangoapps/enrollments/api.py @@ -20,14 +20,14 @@ log = logging.getLogger(__name__) DEFAULT_DATA_API = 'openedx.core.djangoapps.enrollments.data' -def get_enrollments(user_id, include_inactive=False): +def get_enrollments(username, include_inactive=False): """Retrieves all the courses a user is enrolled in. Takes a user and retrieves all relative enrollments. Includes information regarding how the user is enrolled in the the course. Args: - user_id (str): The username of the user we want to retrieve course enrollment information for. + username: The username of the user we want to retrieve course enrollment information for. include_inactive (bool): Determines whether inactive enrollments will be included Returns: @@ -95,16 +95,16 @@ def get_enrollments(user_id, include_inactive=False): ] """ - return _data_api().get_course_enrollments(user_id, include_inactive) + return _data_api().get_course_enrollments(username, include_inactive) -def get_enrollment(user_id, course_id): +def get_enrollment(username, course_id): """Retrieves all enrollment information for the user in respect to a specific course. Gets all the course enrollment information specific to a user in a course. Args: - user_id (str): The user to get course enrollment information for. + username: The user to get course enrollment information for. course_id (str): The course to get enrollment information for. Returns: @@ -142,16 +142,16 @@ def get_enrollment(user_id, course_id): } """ - return _data_api().get_course_enrollment(user_id, course_id) + return _data_api().get_course_enrollment(username, course_id) -def add_enrollment(user_id, course_id, mode=None, is_active=True, enrollment_attributes=None): +def add_enrollment(username, course_id, mode=None, is_active=True, enrollment_attributes=None): """Enrolls a user in a course. Enrolls a user in a course. If the mode is not specified, this will default to `CourseMode.DEFAULT_MODE_SLUG`. Arguments: - user_id (str): The user to enroll. + username: The user to enroll. course_id (str): The course to enroll the user in. mode (str): Optional argument for the type of enrollment to create. Ex. 'audit', 'honor', 'verified', 'professional'. If not specified, this defaults to the default course mode. @@ -196,21 +196,23 @@ def add_enrollment(user_id, course_id, mode=None, is_active=True, enrollment_att if mode is None: mode = _default_course_mode(course_id) validate_course_mode(course_id, mode, is_active=is_active) - enrollment = _data_api().create_course_enrollment(user_id, course_id, mode, is_active) + enrollment = _data_api().create_course_enrollment(username, course_id, mode, is_active) if enrollment_attributes is not None: - set_enrollment_attributes(user_id, course_id, enrollment_attributes) + set_enrollment_attributes(username, course_id, enrollment_attributes) return enrollment -def update_enrollment(user_id, course_id, mode=None, is_active=None, enrollment_attributes=None, include_expired=False): +def update_enrollment( + username, course_id, mode=None, is_active=None, enrollment_attributes=None, include_expired=False +): """Updates the course mode for the enrolled user. Update a course enrollment for the given user and course. Arguments: - user_id (str): The user associated with the updated enrollment. + username: The user associated with the updated enrollment. course_id (str): The course associated with the updated enrollment. Keyword Arguments: @@ -255,22 +257,22 @@ def update_enrollment(user_id, course_id, mode=None, is_active=None, enrollment_ """ log.info(u'Starting Update Enrollment process for user {user} in course {course} to mode {mode}'.format( - user=user_id, + user=username, course=course_id, mode=mode, )) if mode is not None: validate_course_mode(course_id, mode, is_active=is_active, include_expired=include_expired) - enrollment = _data_api().update_course_enrollment(user_id, course_id, mode=mode, is_active=is_active) + enrollment = _data_api().update_course_enrollment(username, course_id, mode=mode, is_active=is_active) if enrollment is None: - msg = u"Course Enrollment not found for user {user} in course {course}".format(user=user_id, course=course_id) + msg = u"Course Enrollment not found for user {user} in course {course}".format(user=username, course=course_id) log.warn(msg) raise errors.EnrollmentNotFoundError(msg) else: if enrollment_attributes is not None: - set_enrollment_attributes(user_id, course_id, enrollment_attributes) + set_enrollment_attributes(username, course_id, enrollment_attributes) log.info(u'Course Enrollment updated for user {user} in course {course} to mode {mode}'.format( - user=user_id, + user=username, course=course_id, mode=mode )) @@ -346,13 +348,13 @@ def get_course_enrollment_details(course_id, include_expired=False): return course_enrollment_details -def set_enrollment_attributes(user_id, course_id, attributes): +def set_enrollment_attributes(username, course_id, attributes): """Set enrollment attributes for the enrollment of given user in the course provided. Args: - course_id (str): The Course to set enrollment attributes for. - user_id (str): The User to set enrollment attributes for. + course_id: The Course to set enrollment attributes for. + username: The User to set enrollment attributes for. attributes (list): Attributes to be set. Example: @@ -368,15 +370,15 @@ def set_enrollment_attributes(user_id, course_id, attributes): ] ) """ - _data_api().add_or_update_enrollment_attr(user_id, course_id, attributes) + _data_api().add_or_update_enrollment_attr(username, course_id, attributes) -def get_enrollment_attributes(user_id, course_id): +def get_enrollment_attributes(username, course_id): """Retrieve enrollment attributes for given user for provided course. Args: - user_id: The User to get enrollment attributes for - course_id (str): The Course to get enrollment attributes for. + username: The User to get enrollment attributes for + course_id: The Course to get enrollment attributes for. Example: >>>get_enrollment_attributes("Bob", "course-v1-edX-DemoX-1T2015") @@ -390,7 +392,7 @@ def get_enrollment_attributes(user_id, course_id): Returns: list """ - return _data_api().get_enrollment_attributes(user_id, course_id) + return _data_api().get_enrollment_attributes(username, course_id) def _default_course_mode(course_id): @@ -459,22 +461,22 @@ def validate_course_mode(course_id, mode, is_active=None, include_expired=False) raise errors.CourseModeNotFoundError(msg, course_enrollment_info) -def unenroll_user_from_all_courses(user_id): +def unenroll_user_from_all_courses(username): """ Unenrolls a specified user from all of the courses they are currently enrolled in. - :param user_id: The id of the user being unenrolled. + :param username: The id of the user being unenrolled. :return: The IDs of all of the organizations from which the learner was unenrolled. """ - return _data_api().unenroll_user_from_all_courses(user_id) + return _data_api().unenroll_user_from_all_courses(username) -def get_user_roles(user_id): +def get_user_roles(username): """ Returns a list of all roles that this user has. - :param user_id: The id of the selected user. + :param username: The id of the selected user. :return: All roles for all courses that this user has. """ - return _data_api().get_user_roles(user_id) + return _data_api().get_user_roles(username) def _data_api(): diff --git a/openedx/core/djangoapps/enrollments/data.py b/openedx/core/djangoapps/enrollments/data.py index c8021c6777..1170f66f0a 100644 --- a/openedx/core/djangoapps/enrollments/data.py +++ b/openedx/core/djangoapps/enrollments/data.py @@ -34,13 +34,13 @@ from student.roles import RoleCache log = logging.getLogger(__name__) -def get_course_enrollments(user_id, include_inactive=False): +def get_course_enrollments(username, include_inactive=False): """Retrieve a list representing all aggregated data for a user's course enrollments. Construct a representation of all course enrollment data for a specific user. Args: - user_id (str): The name of the user to retrieve course enrollment information for. + username: The name of the user to retrieve course enrollment information for. include_inactive (bool): Determines whether inactive enrollments will be included @@ -49,7 +49,7 @@ def get_course_enrollments(user_id, include_inactive=False): """ qset = CourseEnrollment.objects.filter( - user__username=user_id, + user__username=username, ).order_by('created') if not include_inactive: @@ -71,7 +71,7 @@ def get_course_enrollments(user_id, include_inactive=False): ( u"Course enrollments for user %s reference " u"courses that do not exist (this can occur if a course is deleted)." - ), user_id, + ), username, ) return valid @@ -191,13 +191,13 @@ def update_course_enrollment(username, course_id, mode=None, is_active=None): return None -def add_or_update_enrollment_attr(user_id, course_id, attributes): +def add_or_update_enrollment_attr(username, course_id, attributes): """Set enrollment attributes for the enrollment of given user in the course provided. Args: course_id (str): The Course to set enrollment attributes for. - user_id (str): The User to set enrollment attributes for. + username: The User to set enrollment attributes for. attributes (list): Attributes to be set. Example: @@ -214,17 +214,17 @@ def add_or_update_enrollment_attr(user_id, course_id, attributes): ) """ course_key = CourseKey.from_string(course_id) - user = _get_user(user_id) + user = _get_user(username) enrollment = CourseEnrollment.get_enrollment(user, course_key) if not _invalid_attribute(attributes) and enrollment is not None: CourseEnrollmentAttribute.add_enrollment_attr(enrollment, attributes) -def get_enrollment_attributes(user_id, course_id): +def get_enrollment_attributes(username, course_id): """Retrieve enrollment attributes for given user for provided course. Args: - user_id: The User to get enrollment attributes for + username: The User to get enrollment attributes for course_id (str): The Course to get enrollment attributes for. Example: @@ -240,18 +240,18 @@ def get_enrollment_attributes(user_id, course_id): Returns: list """ course_key = CourseKey.from_string(course_id) - user = _get_user(user_id) + user = _get_user(username) enrollment = CourseEnrollment.get_enrollment(user, course_key) return CourseEnrollmentAttribute.get_enrollment_attributes(enrollment) -def unenroll_user_from_all_courses(user_id): +def unenroll_user_from_all_courses(username): """ Set all of a user's enrollments to inactive. - :param user_id: The user being unenrolled. + :param username: The user being unenrolled. :return: A list of all courses from which the user was unenrolled. """ - user = _get_user(user_id) + user = _get_user(username) enrollments = CourseEnrollment.objects.filter(user=user) with transaction.atomic(): for enrollment in enrollments: @@ -260,18 +260,18 @@ def unenroll_user_from_all_courses(user_id): return set([str(enrollment.course_id.org) for enrollment in enrollments]) -def _get_user(user_id): - """Retrieve user with provided user_id +def _get_user(username): + """Retrieve user with provided username Args: - user_id(str): username of the user for which object is to retrieve + username: username of the user for which object is to retrieve Returns: obj """ try: - return User.objects.get(username=user_id) + return User.objects.get(username=username) except User.DoesNotExist: - msg = u"Not user with username '{username}' found.".format(username=user_id) + msg = u"Not user with username '{username}' found.".format(username=username) log.warn(msg) raise UserNotFoundError(msg) @@ -286,7 +286,7 @@ def _invalid_attribute(attributes): """Validate enrollment attribute Args: - attributes(dict): dict of attribute + attributes(List): List of attribute dicts Return: list of invalid attributes @@ -342,14 +342,14 @@ def get_course_enrollment_info(course_id, include_expired=False): return CourseSerializer(course, include_expired=include_expired).data -def get_user_roles(user_id): +def get_user_roles(username): """ Returns a list of all roles that this user has. - :param user_id: The id of the selected user. + :param username: The id of the selected user. :return: All roles for all courses that this user has. """ # pylint: disable=protected-access - user = _get_user(user_id) + user = _get_user(username) if not hasattr(user, '_roles'): user._roles = RoleCache(user) role_cache = user._roles