From 458e8482ea3816ea8b0d2bf3a14506f45b4a0f28 Mon Sep 17 00:00:00 2001 From: Waheed Ahmed Date: Fri, 8 Mar 2019 17:05:25 +0500 Subject: [PATCH] Fix change enrollment command for credit. Get provider id from ecommerce upon changing enrollment to credit instead of setting course_key org. LEARNER-6643 --- .../management/commands/change_enrollment.py | 27 +++++++++------ .../djangoapps/student/tests/test_credit.py | 2 +- common/djangoapps/student/views/dashboard.py | 4 +-- openedx/core/djangoapps/credit/email_utils.py | 33 ++++++++++--------- .../core/djangoapps/credit/tests/test_api.py | 22 +++++++------ 5 files changed, 50 insertions(+), 38 deletions(-) diff --git a/common/djangoapps/student/management/commands/change_enrollment.py b/common/djangoapps/student/management/commands/change_enrollment.py index 45ad35440f..0918e69dd9 100644 --- a/common/djangoapps/student/management/commands/change_enrollment.py +++ b/common/djangoapps/student/management/commands/change_enrollment.py @@ -8,9 +8,8 @@ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey from six import text_type -from student.models import CourseEnrollment, User - -from student.models import CourseEnrollmentAttribute +from openedx.core.djangoapps.credit.email_utils import get_credit_provider_attribute_values +from student.models import CourseEnrollment, CourseEnrollmentAttribute, User logger = logging.getLogger(__name__) # pylint: disable=invalid-name @@ -102,6 +101,17 @@ class Command(BaseCommand): """ Update enrollments for a specific user identifier (email or username). """ users = options[identifier].split(",") + credit_provider_attr = {} + if options['to_mode'] == 'credit': + provider_ids = get_credit_provider_attribute_values( + enrollment_args.get('course_id'), 'id' + ) + credit_provider_attr = { + 'namespace': 'credit', + 'name': 'provider_id', + 'value': provider_ids[0], + } + for identified_user in users: logger.info(identified_user) @@ -119,13 +129,10 @@ class Command(BaseCommand): enrollment.update_enrollment(mode=options['to_mode']) enrollment.save() if options['to_mode'] == 'credit': - enrollment_attrs.append({ - 'namespace': 'credit', - 'name': 'provider_id', - 'value': enrollment_args['course_id'].org, - }) - CourseEnrollmentAttribute.add_enrollment_attr(enrollment=enrollment, - data_list=enrollment_attrs) + enrollment_attrs.append(credit_provider_attr) + CourseEnrollmentAttribute.add_enrollment_attr( + enrollment=enrollment, data_list=enrollment_attrs + ) if options['noop']: raise RollbackException('Forced rollback.') diff --git a/common/djangoapps/student/tests/test_credit.py b/common/djangoapps/student/tests/test_credit.py index ad47f09dfa..9b9ece7169 100644 --- a/common/djangoapps/student/tests/test_credit.py +++ b/common/djangoapps/student/tests/test_credit.py @@ -233,7 +233,7 @@ class CreditCourseDashboardTest(ModuleStoreTestCase): self._make_eligible() # The user should have the option to purchase credit - with patch('student.views.dashboard.get_credit_provider_display_names') as mock_method: + with patch('student.views.dashboard.get_credit_provider_attribute_values') as mock_method: mock_method.return_value = providers_list response = self._load_dashboard() diff --git a/common/djangoapps/student/views/dashboard.py b/common/djangoapps/student/views/dashboard.py index 15fb9dc1c5..eed86a24ef 100644 --- a/common/djangoapps/student/views/dashboard.py +++ b/common/djangoapps/student/views/dashboard.py @@ -33,7 +33,7 @@ from openedx.core.djangoapps.catalog.utils import ( get_pseudo_session_for_entitlement, get_visible_sessions_for_entitlement ) -from openedx.core.djangoapps.credit.email_utils import get_credit_provider_display_names, make_providers_strings +from openedx.core.djangoapps.credit.email_utils import get_credit_provider_attribute_values, make_providers_strings from openedx.core.djangoapps.programs.models import ProgramsApiConfig from openedx.core.djangoapps.programs.utils import ProgramDataExtender, ProgramProgressMeter from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers @@ -483,7 +483,7 @@ def _credit_statuses(user, course_enrollments): statuses = {} for eligibility in credit_api.get_eligibilities_for_user(user.username): course_key = CourseKey.from_string(text_type(eligibility["course_key"])) - providers_names = get_credit_provider_display_names(course_key) + providers_names = get_credit_provider_attribute_values(course_key, 'display_name') status = { "course_key": text_type(course_key), "eligible": True, diff --git a/openedx/core/djangoapps/credit/email_utils.py b/openedx/core/djangoapps/credit/email_utils.py index 4c418774f7..bf2fc7c071 100644 --- a/openedx/core/djangoapps/credit/email_utils.py +++ b/openedx/core/djangoapps/credit/email_utils.py @@ -66,7 +66,7 @@ def send_credit_notifications(username, course_key): # strip enclosing angle brackets from 'logo_image' cache 'Content-ID' logo_image_id = logo_image.get('Content-ID', '')[1:-1] - providers_names = get_credit_provider_display_names(course_key) + providers_names = get_credit_provider_attribute_values(course_key, 'display_name') providers_string = make_providers_strings(providers_names) context = { 'full_name': user.get_full_name(), @@ -196,40 +196,43 @@ def _email_url_parser(url_name, extra_param=None): return urlparse.urlunparse(dashboard_link_parts) -def get_credit_provider_display_names(course_key): +def get_credit_provider_attribute_values(course_key, attribute_name): """Get the course information from ecommerce and parse the data to get providers. Arguments: course_key (CourseKey): The identifier for the course. + attribute_name (String): Name of the attribute of credit provider. Returns: - List of credit provider display names. + List of provided credit provider attribute values. """ course_id = unicode(course_key) credit_config = CreditConfig.current() cache_key = None - provider_names = None + attribute_values = None if credit_config.is_cache_enabled: - cache_key = '{key_prefix}.{course_key}'.format( - key_prefix=credit_config.CACHE_KEY, course_key=course_id + cache_key = '{key_prefix}.{course_key}.{attribute_name}'.format( + key_prefix=credit_config.CACHE_KEY, + course_key=course_id, + attribute_name=attribute_name ) - provider_names = cache.get(cache_key) + attribute_values = cache.get(cache_key) - if provider_names is not None: - return provider_names + if attribute_values is not None: + return attribute_values try: user = User.objects.get(username=settings.ECOMMERCE_SERVICE_WORKER_USERNAME) response = ecommerce_api_client(user).courses(course_id).get(include_products=1) except Exception: # pylint: disable=broad-except log.exception(u"Failed to receive data from the ecommerce course API for Course ID '%s'.", course_id) - return provider_names + return attribute_values if not response: log.info(u"No Course information found from ecommerce API for Course ID '%s'.", course_id) - return provider_names + return attribute_values provider_ids = [] for product in response.get('products'): @@ -237,16 +240,16 @@ def get_credit_provider_display_names(course_key): attr.get('value') for attr in product.get('attribute_values') if attr.get('name') == 'credit_provider' ] - provider_names = [] + attribute_values = [] credit_providers = CreditProvider.get_credit_providers() for provider in credit_providers: if provider['id'] in provider_ids: - provider_names.append(provider['display_name']) + attribute_values.append(provider[attribute_name]) if credit_config.is_cache_enabled: - cache.set(cache_key, provider_names, credit_config.cache_ttl) + cache.set(cache_key, attribute_values, credit_config.cache_ttl) - return provider_names + return attribute_values def make_providers_strings(providers): diff --git a/openedx/core/djangoapps/credit/tests/test_api.py b/openedx/core/djangoapps/credit/tests/test_api.py index 77ef4454e6..4a539ad9a9 100644 --- a/openedx/core/djangoapps/credit/tests/test_api.py +++ b/openedx/core/djangoapps/credit/tests/test_api.py @@ -17,7 +17,7 @@ from opaque_keys.edx.keys import CourseKey from course_modes.models import CourseMode from lms.djangoapps.commerce.tests import TEST_API_URL from openedx.core.djangoapps.credit import api -from openedx.core.djangoapps.credit.email_utils import get_credit_provider_display_names, make_providers_strings +from openedx.core.djangoapps.credit.email_utils import get_credit_provider_attribute_values, make_providers_strings from openedx.core.djangoapps.credit.exceptions import ( CreditRequestNotFound, InvalidCreditCourse, @@ -845,7 +845,9 @@ class CreditRequirementApiTests(CreditApiTestBase): requirements[0]["name"] ) # Satisfy the other requirement. And mocked the api to return different kind of data. - with mock.patch('openedx.core.djangoapps.credit.email_utils.get_credit_provider_display_names') as mock_method: + with mock.patch( + 'openedx.core.djangoapps.credit.email_utils.get_credit_provider_attribute_values' + ) as mock_method: mock_method.return_value = providers_list api.set_credit_requirement_status( user, @@ -1231,7 +1233,7 @@ class CourseApiTests(CreditApiTestBase): def test_get_credit_provider_display_names_method(self): """Verify that parsed providers list is returns after getting course production information.""" self._mock_ecommerce_courses_api(self.course_key, self.COURSE_API_RESPONSE) - response_providers = get_credit_provider_display_names(self.course_key) + response_providers = get_credit_provider_attribute_values(self.course_key, 'display_name') self.assertListEqual(self.PROVIDERS_LIST, response_providers) @httpretty.activate @@ -1239,7 +1241,7 @@ class CourseApiTests(CreditApiTestBase): def test_get_credit_provider_display_names_method_with_exception(self, mock_init): """Verify that in case of any exception it logs the error and return.""" mock_init.side_effect = Exception - response = get_credit_provider_display_names(self.course_key) + response = get_credit_provider_attribute_values(self.course_key, 'display_name') self.assertTrue(mock_init.called) self.assertEqual(response, None) @@ -1250,11 +1252,11 @@ class CourseApiTests(CreditApiTestBase): self._mock_ecommerce_courses_api(self.course_key, self.COURSE_API_RESPONSE) # Warm up the cache. - response_providers = get_credit_provider_display_names(self.course_key) + response_providers = get_credit_provider_attribute_values(self.course_key, 'display_name') self.assertListEqual(self.PROVIDERS_LIST, response_providers) # Hit the cache. - response_providers = get_credit_provider_display_names(self.course_key) + response_providers = get_credit_provider_attribute_values(self.course_key, 'display_name') self.assertListEqual(self.PROVIDERS_LIST, response_providers) # Verify only one request was made. @@ -1269,10 +1271,10 @@ class CourseApiTests(CreditApiTestBase): self._mock_ecommerce_courses_api(self.course_key, self.COURSE_API_RESPONSE) - response_providers = get_credit_provider_display_names(self.course_key) + response_providers = get_credit_provider_attribute_values(self.course_key, 'display_name') self.assertListEqual(self.PROVIDERS_LIST, response_providers) - response_providers = get_credit_provider_display_names(self.course_key) + response_providers = get_credit_provider_attribute_values(self.course_key, 'display_name') self.assertListEqual(self.PROVIDERS_LIST, response_providers) self.assertEqual(len(httpretty.httpretty.latest_requests), 2) @@ -1309,7 +1311,7 @@ class CourseApiTests(CreditApiTestBase): @ddt.unpack def test_get_provider_api_with_multiple_data(self, data, expected_data): self._mock_ecommerce_courses_api(self.course_key, data) - response_providers = get_credit_provider_display_names(self.course_key) + response_providers = get_credit_provider_attribute_values(self.course_key, 'display_name') self.assertEqual(expected_data, response_providers) @httpretty.activate @@ -1317,7 +1319,7 @@ class CourseApiTests(CreditApiTestBase): """Verify that if all providers are in-active than method return empty list.""" self._mock_ecommerce_courses_api(self.course_key, self.COURSE_API_RESPONSE) CreditProvider.objects.all().update(active=False) - self.assertEqual(get_credit_provider_display_names(self.course_key), []) + self.assertEqual(get_credit_provider_attribute_values(self.course_key, 'display_name'), []) @ddt.data(None, ['asu'], ['asu', 'co'], ['asu', 'co', 'mit']) def test_make_providers_strings(self, providers):